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

[Alex G.]

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

Alex