[U-Boot] [RFC PATCH] tools/imagetool: remove linker generated list
Simon Glass
sjg at chromium.org
Tue Feb 10 16:01:17 CET 2015
Hi Andreas,
On 7 February 2015 at 14:19, Andreas Bießmann
<andreas.devel at 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 at googlemail.com>
> Cc: Guilherme Maciel Ferreira <guilherme.maciel.ferreira at 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
More information about the U-Boot
mailing list