On 5/13/21 6:56 PM, Simon Glass wrote:
Hi Alex,
On Thu, 13 May 2021 at 10:21, Alex G. <mr.nuke...@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...@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...@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...@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.
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.
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.
Regards,
Simon
