[U-Boot] [RFC PATCH] tools/imagetool: remove linker generated list

[Simon Glass <sjg@chromium.org>]

Hi Andreas,

On 7 February 2015 at 14:19, Andreas Bie?mann
<andreas.devel@googlemail.com> wrote:
> Commit a93648d197df48fa46dd55f925ff70468bd81c71 introduced linker generated
> lists for imagetool which is part of mkimage. It is a nice feature to remove
> the annoying register function calls, but is not portable. Unfortunately some
> host compilers do not support this type of linker scripts. Therefore this
> commit broke this host-tool for theem, namely FreeBSD and Darwin (OS/X).
>
> This commit tries to fix this. We won't go back to the register functions but
> we also can not use the linker script. So use another approach copied from
> linux kernel scripts/mod/file2alias.c.
>
> Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
> Cc: Guilherme Maciel Ferreira <guilherme.maciel.ferreira@gmail.com>
> ---
>
>  tools/Makefile      |    2 --
>  tools/imagetool.c   |   34 +++++++++++++++----------------
>  tools/imagetool.h   |   56 +++++++++++++++++++++++++++++++++++++++++----------
>  tools/imagetool.lds |   24 ----------------------
>  4 files changed, 61 insertions(+), 55 deletions(-)
>  delete mode 100644 tools/imagetool.lds

I have a FreeBSD set up now thanks to Jeroen so have had a play with this.

>
> diff --git a/tools/Makefile b/tools/Makefile
> index 6e1ce79..ea76a3e 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -124,8 +124,6 @@ HOSTLOADLIBES_dumpimage := $(HOSTLOADLIBES_mkimage)
>  HOSTLOADLIBES_fit_info := $(HOSTLOADLIBES_mkimage)
>  HOSTLOADLIBES_fit_check_sign := $(HOSTLOADLIBES_mkimage)
>
> -HOSTLDFLAGS += -T $(srctree)/tools/imagetool.lds
> -
>  hostprogs-$(CONFIG_EXYNOS5250) += mkexynosspl
>  hostprogs-$(CONFIG_EXYNOS5420) += mkexynosspl
>  HOSTCFLAGS_mkexynosspl.o := -pedantic
> diff --git a/tools/imagetool.c b/tools/imagetool.c
> index 148e466..8563032 100644
> --- a/tools/imagetool.c
> +++ b/tools/imagetool.c
> @@ -12,16 +12,15 @@
>
>  struct image_type_params *imagetool_get_type(int type)
>  {
> -       struct image_type_params *curr;
> -       struct image_type_params *start = ll_entry_start(
> -                       struct image_type_params, image_type);
> -       struct image_type_params *end = ll_entry_end(
> -                       struct image_type_params, image_type);
> +       struct image_type_params **curr;
> +       INIT_SECTION(image_type);
> +       struct image_type_params **start = __start_image_type;
> +       struct image_type_params **end = __stop_image_type;
>
>         for (curr = start; curr != end; curr++) {
> -               if (curr->check_image_type) {
> -                       if (!curr->check_image_type(type))
> -                               return curr;
> +               if ((*curr)->check_image_type) {
> +                       if (!(*curr)->check_image_type(type))
> +                               return *curr;
>                 }
>         }
>         return NULL;
> @@ -34,16 +33,15 @@ int imagetool_verify_print_header(
>         struct image_tool_params *params)
>  {
>         int retval = -1;
> -       struct image_type_params *curr;
> +       struct image_type_params **curr;
> +       INIT_SECTION(image_type);
>
> -       struct image_type_params *start = ll_entry_start(
> -                       struct image_type_params, image_type);
> -       struct image_type_params *end = ll_entry_end(
> -                       struct image_type_params, image_type);
> +       struct image_type_params **start = __start_image_type;
> +       struct image_type_params **end = __stop_image_type;
>
>         for (curr = start; curr != end; curr++) {
> -               if (curr->verify_header) {
> -                       retval = curr->verify_header((unsigned char *)ptr,
> +               if ((*curr)->verify_header) {
> +                       retval = (*curr)->verify_header((unsigned char *)ptr,
>                                                      sbuf->st_size, params);
>
>                         if (retval == 0) {
> @@ -51,12 +49,12 @@ int imagetool_verify_print_header(
>                                  * Print the image information  if verify is
>                                  * successful
>                                  */
> -                               if (curr->print_header) {
> -                                       curr->print_header(ptr);
> +                               if ((*curr)->print_header) {
> +                                       (*curr)->print_header(ptr);
>                                 } else {
>                                         fprintf(stderr,
>                                                 "%s: print_header undefined for %s\n",
> -                                               params->cmdname, curr->name);
> +                                               params->cmdname, (*curr)->name);
>                                 }
>                                 break;
>                         }
> diff --git a/tools/imagetool.h b/tools/imagetool.h
> index f35dec7..3e15b4e 100644
> --- a/tools/imagetool.h
> +++ b/tools/imagetool.h
> @@ -20,15 +20,6 @@
>  #include <unistd.h>
>  #include <u-boot/sha1.h>
>
> -/* define __KERNEL__ in order to get the definitions
> - * required by the linker list. This is probably not
> - * the best way to do this */
> -#ifndef __KERNEL__
> -#define __KERNEL__
> -#include <linker_lists.h>
> -#undef __KERNEL__
> -#endif /* __KERNEL__ */
> -
>  #include "fdt_host.h"
>
>  #define ARRAY_SIZE(x)          (sizeof(x) / sizeof((x)[0]))
> @@ -194,6 +185,46 @@ int imagetool_save_subimage(
>
>  void pbl_load_uboot(int fd, struct image_tool_params *mparams);
>
> +#define ___cat(a, b) a ## b
> +#define __cat(a, b) ___cat(a, b)
> +
> +/* we need some special handling for this host tool running eventually on
> + * Darwin. The Mach-O section handling is a bit different than ELF section
> + * handling. The differnces in detail are:
> + *  a) we have segments which have sections
> + *  b) we need a API call to get the respective section symbols */
> +#if defined(__MACH__)
> +#include <mach-o/getsect.h>
> +
> +#define INIT_SECTION(name)  do {                                       \
> +               unsigned long name ## _len;                             \
> +               char *__cat(pstart_, name) = getsectdata("__TEXT",      \
> +                       #name, &__cat(name, _len));                     \
> +               char *__cat(pstop_, name) = __cat(pstart_, name) +      \
> +                       __cat(name, _len);                              \
> +               __cat(__start_, name) = (void *)__cat(pstart_, name);   \
> +               __cat(__stop_, name) = (void *)__cat(pstop_, name);     \
> +       } while (0)
> +#define SECTION(name)   __attribute__((section("__TEXT, " #name)))

If I understand this correctly, in both cases we put things in a
separate section so that all the pieces get collected together. The
only difference seems to be the naming of the section - for MACH it
has a __TEXT prefix. Is that right? If so, I wonder if this should go
in linker_list.h?

For access to the data, here we are using a list of pointers to the
struct rather than a list of struct. Why is that? I can't see that
this is required by the MACH system.

If that is correct, then it should be easy to support linker_list on MACH, by:

- Adding a __TEXT prefix to all the section() bits in linker_lists.h
- Changing any access to the lists to use INIT_SECTION instead of
going there directly

Then we should be able to support running sandbox.

Does that sound feasible?

> +
> +struct image_type_params **__start_image_type, **__stop_image_type;
> +#else
> +#define INIT_SECTION(name) /* no-op for ELF */
> +#define SECTION(name)   __attribute__((section(#name)))
> +
> +/* We construct a table of pointers in an ELF section (pointers generally
> + * go unpadded by gcc).  ld creates boundary syms for us. */
> +extern struct image_type_params *__start_image_type[], *__stop_image_type[];
> +#endif /* __MACH__ */
> +
> +#if !defined(__used)
> +# if __GNUC__ == 3 && __GNUC_MINOR__ < 3
> +#  define __used                       __attribute__((__unused__))
> +# else
> +#  define __used                       __attribute__((__used__))
> +# endif
> +#endif
> +
>  #define U_BOOT_IMAGE_TYPE( \
>                 _id, \
>                 _name, \
> @@ -208,7 +239,8 @@ void pbl_load_uboot(int fd, struct image_tool_params *mparams);
>                 _fflag_handle, \
>                 _vrec_header \
>         ) \
> -       ll_entry_declare(struct image_type_params, _id, image_type) = { \
> +       static struct image_type_params __cat(image_type_, _id) = \
> +       { \
>                 .name = _name, \
>                 .header_size = _header_size, \
>                 .hdr = _header, \
> @@ -220,6 +252,8 @@ void pbl_load_uboot(int fd, struct image_tool_params *mparams);
>                 .check_image_type = _check_image_type, \
>                 .fflag_handle = _fflag_handle, \
>                 .vrec_header = _vrec_header \
> -       }
> +       }; \
> +       static struct image_type_params *SECTION(image_type) __used \
> +               __cat(image_type_ptr_, _id) = &__cat(image_type_, _id)
>
>  #endif /* _IMAGETOOL_H_ */
> diff --git a/tools/imagetool.lds b/tools/imagetool.lds
> deleted file mode 100644
> index 7e92b4a..0000000
> --- a/tools/imagetool.lds
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -/*
> - * Copyright (c) 2011-2012 The Chromium OS Authors.
> - * Use of this source code is governed by a BSD-style license that can be
> - * found in the LICENSE file.
> - *
> - * SPDX-License-Identifier:    GPL-2.0+
> - */
> -
> -SECTIONS
> -{
> -
> -       . = ALIGN(4);
> -       .u_boot_list : {
> -               KEEP(*(SORT(.u_boot_list*)));
> -       }
> -
> -       __u_boot_sandbox_option_start = .;
> -       _u_boot_sandbox_getopt : { *(.u_boot_sandbox_getopt) }
> -       __u_boot_sandbox_option_end = .;
> -
> -       __bss_start = .;
> -}
> -
> -INSERT BEFORE .data;
> --
> 1.7.10.4
>

Regards,
Simon