From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex G. Date: Thu, 13 May 2021 11:21:48 -0500 Subject: [PATCH v2 16/50] image: Add Kconfig options for FIT in the host build In-Reply-To: References: <20210506142438.1310977-1-sjg@chromium.org> <20210506082420.v2.16.I64826ed33219988294468df7b95dfa3fffd7a0a1@changeid> <659c76d0-d9aa-e270-0eb8-25cefdc238e9@gmail.com> <46a12083-b684-ff5e-7552-9fecb6e8da5b@gmail.com> Message-ID: <07146d8e-c674-aa34-3f5b-f3bc53489c3e@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 5/12/21 12:30 PM, Simon Glass wrote: > Hi Alex, > > On Wed, 12 May 2021 at 10:18, Alex G. wrote: >> >> >> >> On 5/12/21 10:54 AM, Simon Glass wrote: >>> Hi Alex, >>> >>> On Wed, 12 May 2021 at 09:48, Alex G. wrote: >>>> >>>> >>>> >>>> On 5/12/21 9:51 AM, Simon Glass wrote: >>>>> Hi Alex, >>>>> >>>>> On Tue, 11 May 2021 at 13:57, Alex G. wrote: >>>>>> >>>>>> On 5/6/21 9:24 AM, Simon Glass wrote: >>>> >>>> [snip] >>>> >>>>>> >>>>>>> + >>>>>>> +config HOST_FIT_PRINT >>>>>>> + def_bool y >>>>>>> + help >>>>>>> + Print the content of the FIT verbosely in the host build >>>>>> >>>>>> This option also doesn't make sense.This seems to do what 'mkimage -l' >>>>>> already supports. >>>>> >>>>> Are you sure you have looked at the goal of the CONFIG_IS_ENABLED() >>>>> changes? This is here purely to avoid #ifdefs in the share code. >>>> >>>> On the one hand, we have the cosmetic inconvenience caused by #ifdefs. >>>> On the other hand we have the config system. To most users, the config >>>> system is likely more visible, while it's mostly developers who will >>>> ever see the ifdefs. >>>> >>>> Therefore, in order to get the developer convenience of less ifdefs, we >>>> have to sacrifice user convenience by cloberring the Kconfig options. I >>>> think this is back-to-front. >>> >>> These Kconfig options are not visible to users. They cannot be updated >>> in defconfig, nor in 'make menuconfig', etc. They are purely there for >>> the build system. >>> >>>> >>>> Can we reduce the host config count to just SLL/NOSSL? >>> >>> The point here is that the code has a special case for host builds, >>> and this is a means to remove that special case and make the code >>> easier to maintain and follow. >> >> I understand where you're coming from. Without these changes, the code >> knows what it should and should not do, correct? My argument is that if >> the code has the logic to do the correct thing, that logic should not be >> split with the config system. >> >> I agree with the goal of reducing clutter in the source code. I disagree >> with this specific course of fixing it. Instead, I propose a single >> kconfig for host tools for SSL on/off. >> >> The disadvantage of my proposal is that we have to refactor the common >> code in a way consistent with the goals, instead of just changing some >> #ifdefs to if(CONFIG_IS_ENABLED()). I admit that, off the top of my >> head, I don't have a detailed plan on how to achieve this. > > You are mostly describing the status quo, so far as I understand it. > The problem is with the code that is built for both boards and tools. > For boards, we want this code to be compiled conditionally, depending > on what options are enabled. For tools, we want the code to be > compiled unconditionally. > > I can think of only three ways to do this: > > - status quo (add #ifdefs USE_HOSTCC wherever we need to) > - my series (make use of hidden Kconfig options to avoid that) > - put every single feature and associated lines of code in separate > files and compile them conditionally for boards, but always for tools > > I believe the last option is actually impossible, or at least > impractical. It would cause an explosion of source files to deal with > all the various combinations, and would be quite painful to maintain > also. I don't think the status quo is such a terrible solution, so I am looking at the aspects that can benefit from improvement. Hence why it may appear I am talking about the status quo. Let's assume CONFIG_IS_ENABLED() returns true on host builds, and for those cases where you need to know if IS_HOST_BUILD(), there's a macro for that. You have the oddball case that uses a CONFIG_ value that's only defined on the target, and those you would have to split according to your option #3. I don't think the above is as dire an explosion in source files as it seems. Another point of contention is checksum_algos and crypto_algos arrays image-sig.c. On the target side, these should be in .u-boot-list. Status quo is the definition of rsa_verify is hidden behind #if macros, which just pushes the complexity out into the rsa.h headers. The two ideas here are CONFIG_IS_ENABLED() returns true for host code, and image-sig.c is split bwtween host and target versions, the target version making use of .uboot-list. Using these as the starting points, I think we can get to a much better solution Alex