[PATCH] image.h: isolate android_image_* functions from tooling

[PATCH] image.h: isolate android_image_* functions from tooling

[Eugeniu Rosca]

On Feb. 16, 2020, Tom reported [1] build failure of U-Boot in-tree
tooling after applying https://patchwork.ozlabs.org/cover/1229663/
("[v6,0/7] rsa: extend rsa_verify() for UEFI secure boot").

Later on, Heinrich stressed the urgency of the issue in
https://patchwork.ozlabs.org/patch/1250858/#2379069:

 >>>>>>>>>
 We should finalize the topic as it stops EFI patches from being merged
 >>>>>>>>>

On the surface, the problem is caused by U-Boot commits [2-3], which
employed 'u32' in 'include/image.h', while historically U-Boot tooling
stayed agnostic on the {u,s}{8,16,32} types.

Thanks to Tom, Yamada-san and Heinrich, the following solutions have
been put head-to-head ('+' pros, '-' cons):

 A. Use an equivalent fixed-size type, i.e. s/u32/uint32_t/ in both
    android function prototypes (image.h) and definitions (c file):
    + quick and low-line-count
    - creates a 'soup' of fixed-sized types in the Android C file
    - will confuse contributors
    - is going against Linux kernel best practices [4]

 B. Guard Android functions by '!defined(USE_HOSTCC)' in image.h:
    + quick and low-line-count
    + reflects the reality (no android function is used by tooling)
    + zero impact on other subsystems
    - ifdeffery may look annoying (pre-existing problem of image.h)

 C. Make {u8,u16,u32} available in U-Boot tooling:
    + quick and low-line-count
    + [Yamada-san][5]:
      * forbidding u32 for tools is questionable to me
      * Linux kernel and Barebox use {u8,u16,u32} for the tools space
    - breaks U-Boot tradition?
    - has larger impact than [A] and [B]
    - adds type complexity/inconsistency in the tooling space

 D. [Yamada-san] Refactor the headers to minimize the code shared
    between U-Boot space and tooling space:
    + probably the long-term solution
    - high effort
    - can be seen/done as an incremental update on top of [B]

Looking at the above, [B] looks like the natural way to go forward.

[1] https://patchwork.ozlabs.org/patch/1238245/#2363052
[2] commit 7f2531502c74c0 ("image: android: Add routine to get dtbo params")
[3] commit c3bfad825a71ea ("image: android: Add functions for handling dtb field")
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e6176fa4728fb6d
    ("checkpatch: add --strict warning for c99 fixed size typedefs : int<size>_t")
[5] https://patchwork.ozlabs.org/patch/1238245/#2363340

Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Sam Protsenko <joe.skb7@gmail.com>
Cc: Lokesh Vutla <lokeshvutla@ti.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reported-by: Tom Rini <trini@konsulko.com>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 include/image.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/image.h b/include/image.h
index b316d167d8d7..645daeea507b 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1416,6 +1416,7 @@ struct cipher_algo *image_get_cipher_algo(const char *full_name);
 #endif /* CONFIG_FIT_VERBOSE */
 #endif /* CONFIG_FIT */
 
+#if !defined(USE_HOSTCC)
 #if defined(CONFIG_ANDROID_BOOT_IMAGE)
 struct andr_img_hdr;
 int android_image_check_header(const struct andr_img_hdr *hdr);
@@ -1437,6 +1438,7 @@ bool android_image_print_dtb_contents(ulong hdr_addr);
 #endif
 
 #endif /* CONFIG_ANDROID_BOOT_IMAGE */
+#endif /* !USE_HOSTCC */
 
 /**
  * board_fit_config_name_match() - Check for a matching board name
-- 
2.25.0

[PATCH] image.h: isolate android_image_* functions from tooling

[Heinrich Schuchardt]

On 3/8/20 1:11 AM, Eugeniu Rosca wrote:
> On Feb. 16, 2020, Tom reported [1] build failure of U-Boot in-tree
> tooling after applying https://patchwork.ozlabs.org/cover/1229663/
> ("[v6,0/7] rsa: extend rsa_verify() for UEFI secure boot").
>
> Later on, Heinrich stressed the urgency of the issue in
> https://patchwork.ozlabs.org/patch/1250858/#2379069:
>
>   >>>>>>>>>
>   We should finalize the topic as it stops EFI patches from being merged
>   >>>>>>>>>
>
> On the surface, the problem is caused by U-Boot commits [2-3], which
> employed 'u32' in 'include/image.h', while historically U-Boot tooling
> stayed agnostic on the {u,s}{8,16,32} types.
>
> Thanks to Tom, Yamada-san and Heinrich, the following solutions have
> been put head-to-head ('+' pros, '-' cons):
>
>   A. Use an equivalent fixed-size type, i.e. s/u32/uint32_t/ in both
>      android function prototypes (image.h) and definitions (c file):
>      + quick and low-line-count
>      - creates a 'soup' of fixed-sized types in the Android C file
>      - will confuse contributors
>      - is going against Linux kernel best practices [4]
>
>   B. Guard Android functions by '!defined(USE_HOSTCC)' in image.h:
>      + quick and low-line-count
>      + reflects the reality (no android function is used by tooling)
>      + zero impact on other subsystems
>      - ifdeffery may look annoying (pre-existing problem of image.h)
>
>   C. Make {u8,u16,u32} available in U-Boot tooling:
>      + quick and low-line-count
>      + [Yamada-san][5]:
>        * forbidding u32 for tools is questionable to me
>        * Linux kernel and Barebox use {u8,u16,u32} for the tools space
>      - breaks U-Boot tradition?
>      - has larger impact than [A] and [B]
>      - adds type complexity/inconsistency in the tooling space
>
>   D. [Yamada-san] Refactor the headers to minimize the code shared
>      between U-Boot space and tooling space:
>      + probably the long-term solution
>      - high effort
>      - can be seen/done as an incremental update on top of [B]
>
> Looking at the above, [B] looks like the natural way to go forward.
>
> [1] https://patchwork.ozlabs.org/patch/1238245/#2363052
> [2] commit 7f2531502c74c0 ("image: android: Add routine to get dtbo params")
> [3] commit c3bfad825a71ea ("image: android: Add functions for handling dtb field")
> [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e6176fa4728fb6d
>      ("checkpatch: add --strict warning for c99 fixed size typedefs : int<size>_t")
> [5] https://patchwork.ozlabs.org/patch/1238245/#2363340
>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Sam Protsenko <joe.skb7@gmail.com>
> Cc: Lokesh Vutla <lokeshvutla@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Reported-by: Tom Rini <trini@konsulko.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

The patch is sufficient to overcome the build errors occurring with
Takahiro's RSA and secure boot patches.
https://patchwork.ozlabs.org/project/uboot/list/?series=&submitter=61166

Tested-by: Heinrich Schuchardt <xpyron.glpk@gmx.de>

[PATCH] image.h: isolate android_image_* functions from tooling

[Tom Rini]

On Sun, Mar 08, 2020 at 01:11:18AM +0100, Eugeniu Rosca wrote:

> On Feb. 16, 2020, Tom reported [1] build failure of U-Boot in-tree
> tooling after applying https://patchwork.ozlabs.org/cover/1229663/
> ("[v6,0/7] rsa: extend rsa_verify() for UEFI secure boot").
> 
> Later on, Heinrich stressed the urgency of the issue in
> https://patchwork.ozlabs.org/patch/1250858/#2379069:
> 
>  >>>>>>>>>
>  We should finalize the topic as it stops EFI patches from being merged
>  >>>>>>>>>
> 
> On the surface, the problem is caused by U-Boot commits [2-3], which
> employed 'u32' in 'include/image.h', while historically U-Boot tooling
> stayed agnostic on the {u,s}{8,16,32} types.
> 
> Thanks to Tom, Yamada-san and Heinrich, the following solutions have
> been put head-to-head ('+' pros, '-' cons):
> 
>  A. Use an equivalent fixed-size type, i.e. s/u32/uint32_t/ in both
>     android function prototypes (image.h) and definitions (c file):
>     + quick and low-line-count
>     - creates a 'soup' of fixed-sized types in the Android C file
>     - will confuse contributors
>     - is going against Linux kernel best practices [4]
> 
>  B. Guard Android functions by '!defined(USE_HOSTCC)' in image.h:
>     + quick and low-line-count
>     + reflects the reality (no android function is used by tooling)
>     + zero impact on other subsystems
>     - ifdeffery may look annoying (pre-existing problem of image.h)
> 
>  C. Make {u8,u16,u32} available in U-Boot tooling:
>     + quick and low-line-count
>     + [Yamada-san][5]:
>       * forbidding u32 for tools is questionable to me
>       * Linux kernel and Barebox use {u8,u16,u32} for the tools space
>     - breaks U-Boot tradition?
>     - has larger impact than [A] and [B]
>     - adds type complexity/inconsistency in the tooling space
> 
>  D. [Yamada-san] Refactor the headers to minimize the code shared
>     between U-Boot space and tooling space:
>     + probably the long-term solution
>     - high effort
>     - can be seen/done as an incremental update on top of [B]
> 
> Looking at the above, [B] looks like the natural way to go forward.
> 
> [1] https://patchwork.ozlabs.org/patch/1238245/#2363052
> [2] commit 7f2531502c74c0 ("image: android: Add routine to get dtbo params")
> [3] commit c3bfad825a71ea ("image: android: Add functions for handling dtb field")
> [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e6176fa4728fb6d
>     ("checkpatch: add --strict warning for c99 fixed size typedefs : int<size>_t")
> [5] https://patchwork.ozlabs.org/patch/1238245/#2363340
> 
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Sam Protsenko <joe.skb7@gmail.com>
> Cc: Lokesh Vutla <lokeshvutla@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Reported-by: Tom Rini <trini@konsulko.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Tested-by: Heinrich Schuchardt <xpyron.glpk@gmx.de>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200312/b778ad9d/attachment.sig>