[U-Boot] [PATCH v1] tools/imagetool: remove linker script
Simon Glass
sjg at chromium.org
Tue Feb 10 16:08:15 CET 2015
Hi Andreas,
On 8 February 2015 at 16:06, Andreas Bießmann
<andreas.devel at googlemail.com> wrote:
> Commit a93648d197df48fa46dd55f925ff70468bd81c71 introduced linker generated
> lists for imagetool which is the base for some host tools (mkimage, dumpimage,
> et al.). Unfortunately some host tool chains do not support the used type of
> linker scripts. Therefore this commit broke these host-tools for them, namely
> FreeBSD and Darwin (OS/X).
>
> This commit tries to fix this. In order to have a clean distinction between host
> and embedded code space we need to introduce our own linker generated list
> instead of re-using the available linker_lists.h provided functionality. So we
> copy the implementation used in linux kernel script/mod/file2alias.c which has
> the very same problem (cause it is a host tool). This code also comes with an
> abstraction for Mach-O binary format used in Darwin systems.
>
> Signed-off-by: Andreas Bießmann <andreas.devel at googlemail.com>
> Cc: Guilherme Maciel Ferreira <guilherme.maciel.ferreira at gmail.com>
> ---
> It is highly encouraged to have a clear distinction between host stuff and
> embedded stuff. We have the __KERNEL__/__UBOOT__ preprocessor directive to do
> so, lets respect it and do not pollute the host space with the embedded stuff.
I think this is a reference ot the defining of __KERNEL__ in
imagetool.h. That is apparently needed for linker_lists.h although I
really don't see why. It should be possible to adjust either
compiler.h or linker_lsits.h to work around this.
>
> Wait a minute ... wasn't there a stdint.h discussion these
> days? Isn't this exactly the same thing but the other way round?
I think that is a separate topic and as mentioned there I feel this is
a feature of the toolchain, not the host.
>
> Changes in v1:
> - disable ASLR for host tools on OS X, it generates a linker warning
>
> Makefile | 5 +++++
> tools/Makefile | 2 --
> tools/imagetool.c | 35 ++++++++++++++++----------------
> tools/imagetool.h | 56 +++++++++++++++++++++++++++++++++++++++++----------
> tools/imagetool.lds | 24 ----------------------
> 5 files changed, 67 insertions(+), 55 deletions(-)
> delete mode 100644 tools/imagetool.lds
>
> diff --git a/Makefile b/Makefile
> index 92faed6..aca8587 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -281,6 +281,11 @@ os_x_before = $(shell if [ $(DARWIN_MAJOR_VERSION) -le $(1) -a \
> HOSTCC = $(call os_x_before, 10, 5, "cc", "gcc")
> HOSTCFLAGS += $(call os_x_before, 10, 4, "-traditional-cpp")
> HOSTLDFLAGS += $(call os_x_before, 10, 5, "-multiply_defined suppress")
> +
> +# since Lion (10.7) ASLR is on by default, but we use linker generated lists
> +# in some host tools which is a problem then ... so disable ASLR for these
> +# tools
> +HOSTLDFLAGS += $(call os_x_before, 10, 7, "", "-Xlinker -no_pie")
> endif
>
> # Decide whether to build built-in, modular, or both.
> 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..4b0b73d 100644
> --- a/tools/imagetool.c
> +++ b/tools/imagetool.c
> @@ -12,16 +12,16 @@
>
> 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 +34,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 +50,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)))
> +
> +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__ */
(Repeating as I comment on your RFC patch instead of this)
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?
> +
> +#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