[PATCH v2 16/50] image: Add Kconfig options for FIT in the host build

[Simon Glass]

Hi Alex,

On Fri, 14 May 2021 at 09:12, Alex G. <mr.nuke.me at gmail.com> wrote:
>
>
>
> On 5/13/21 6:56 PM, Simon Glass wrote:
> > Hi Alex,
> >
> > On Thu, 13 May 2021 at 10:21, Alex G. <mr.nuke.me at gmail.com> wrote:
> >>
> >>
> >>
> >> On 5/12/21 12:30 PM, Simon Glass wrote:
> >>> Hi Alex,
> >>>
> >>> On Wed, 12 May 2021 at 10:18, Alex G. <mr.nuke.me at gmail.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 5/12/21 10:54 AM, Simon Glass wrote:
> >>>>> Hi Alex,
> >>>>>
> >>>>> On Wed, 12 May 2021 at 09:48, Alex G. <mr.nuke.me at gmail.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 5/12/21 9:51 AM, Simon Glass wrote:
> >>>>>>> Hi Alex,
> >>>>>>>
> >>>>>>> On Tue, 11 May 2021 at 13:57, Alex G. <mr.nuke.me at gmail.com> 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