From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Sat, 15 May 2021 09:20:22 -0600 Subject: [PATCH v2 16/50] image: Add Kconfig options for FIT in the host build In-Reply-To: <1f47fa1c-22eb-a59a-7144-fe0639b29c56@gmail.com> 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> <07146d8e-c674-aa34-3f5b-f3bc53489c3e@gmail.com> <1f47fa1c-22eb-a59a-7144-fe0639b29c56@gmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Alex, On Fri, 14 May 2021 at 09:12, Alex G. wrote: > > > > On 5/13/21 6:56 PM, Simon Glass wrote: > > Hi Alex, > > > > On Thu, 13 May 2021 at 10:21, Alex G. wrote: > >> > >> > >> > >> 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 > > > > I did consider simply defining CONFIG_IS_ENABLED() to true for the > > host, but rejected that because I worried it would break down at some > > point. Examples I can think of at the moment: > > > > - conflicting or alternative options (OF_LIVE, OF_HOST_FILE, OF_SEPARATE) > > - code that is not actually wanted on the host (WATCHDOG, > > NEEDS_MANUAL_RELOC, FPGA) > > - code that we want on the host but not the board (so we end up with a > > dummy CONFIG for the boards?) > > - all the SPL / TPL / VPL options which would always end up being > > true, when in fact we probably want none of them > > > > I think you should more clearly explain your objection to the hidden > > Kconfig options, since your original reason ("clobbering the Kconfig > > space") doesn't seem to have survived further analysis. > > I thought it to have been already explained and settled. It objectively > increase the complexity of the logic. Instead of the logic being defined > in one place (code), it is now defined in two places (code and Kconfig). > > And subjectively, when adding ECDSA support, I found the ifs/ifdefs that > were derived from kconfig code to be extremely confusing. It would have > helped trememdously if the host code would flow with less 'kconfig' > decision points. I don't find this series to improve on that. I think separating out host code is fine. But we do need to be careful. A few years back someone added fit_check_sign and something else that runs the board code. If we had had separate code at that point, the additional of that tool would have been a huge lift. So I think we should: - separate out cost that is clearly host-only (perhaps into the tools/ dir) - don't separate out board code, since we will likely want to run it with a tool one day > > > Having said that, I can imagine with a lot of refactoring it might be > > possible to address some of those. I just don't see it as a feasible > > option from here. The effort would be better spent improving testing, > > I think. But if you'd like to code something up for it, I'd be > > interested to see it. > > We are in agreement that refactoring will improve the situation. I don't > think it's so dire that it's unfeasible. However, if you'd like, we can > start in smaller chunks. OK. > > > Re the linker-list stuff, yes we should push more things to those > > instead of #ifdefs and weak functions. Hashing and crypto are prime > > examples. In fact host tools can use linker lists too if we need that. > > Pretty low hanging fruit here. Let me try to code something up. OK. Perhaps with some separation, the Kconfig stuff will become easier. Regards, Simon