[PATCH] image.h: isolate android_image_* functions from tooling
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/?i... ("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
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/?i... ("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
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/?i... ("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!
participants (3)
-
Eugeniu Rosca -
Heinrich Schuchardt -
Tom Rini