Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

From: Marek Vasut <marex@denx.de>
To: Tom Rini <trini@konsulko.com>
Cc: u-boot@lists.denx.de, Simon Glass <sjg@chromium.org>,
	Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Subject: Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined
Date: Sat, 4 Sep 2021 16:03:25 +0200	[thread overview]
Message-ID: <678a77d9-356a-e924-da8e-102fba938ee3@denx.de> (raw)
In-Reply-To: <20210830120102.GO858@bill-the-cat>

On 8/30/21 2:01 PM, Tom Rini wrote:

[...]

>>>>>>>>>>>>>>> We shouldn't need this at all.  LMB and LMB_USE_MAX_REGIONS are both in
>>>>>>>>>>>>>>> Kconfig and have the dependencies expressed that way.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be
>>>>>>>>>>>>>> undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four
>>>>>>>>>>>>>> different symbols.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'm still not seeing it, sorry.  Is there some case where we're trying
>>>>>>>>>>>>> to access a struct lmb without CONFIG_LMB enabled?
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> See build failure
>>>>>>>>>>>> https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331
>>>>>>>>>>>
>>>>>>>>>>> Ah, progress.  Drop <lmb.h> from <image.h> since we already have a
>>>>>>>>>>> forward declaration of struct lmb?  But it's not failing without this
>>>>>>>>>>> series too, so what's changing?
>>>>>>>>>>
>>>>>>>>>> See 01/14 in this series.
>>>>>>>>>
>>>>>>>>> Ah, so drop 1/14 then.
>>>>>>>>
>>>>>>>> Why ? That patch is correct.
>>>>>>>
>>>>>>> It's not quite right, 1/14 and then 2/14 are papering over the fact that
>>>>>>> lmb.h, and it's including headers / files, need to be cleaned up so that
>>>>>>> we don't need to have redundant tests in the header.
>>>>>>
>>>>>> 1/14 disables LMB and CMD_BDI for tools build, we do not need those, so 1/14
>>>>>> is correct.
>>>>>
>>>>> We don't need to build u-boot at all for tools-only, only the tools-only
>>>>> build target.  It's just annoying to exclude the tools-only_defconfig from
>>>>> "sandbox" in CI.
>>>>
>>>> So, what exactly is the problem with that 01/14 ? Please elaborate, I
>>>> believe the patch is correct.
>>>
>>> You disable LMB in a target that's only building "all" in CI because
>>> wasn't ever worth adding ",sandbox" to the all other arches job until
>>> perhaps now.
>>>
>>> Disabling LMB in tools-only_defconfig then exposes that <lmb.h> can only
>>> be included safely when CONFIG_LMB is set.
>>>
>>> Adding / extending an #if test in code for something that's already
>>> checked for in Kconfig is bad.  We spent so much time already removing
>>> and shrinking #if tests in the code.
>>
>> So, the patch is correct, the headers need further clean up.
> 
> No, it's not.  The first patch is wrong because disabling CONFIG_LMB is
> invalid.

Please explain why the patch disabling LMB support for tools-only build 
is invalid. I disagree with this statement, LMB support in tools-only 
build is useless, because LMB protects parts of running U-Boot from 
being overwritten.

> The second patch is conceptually wrong because if we're
> enforcing a check in C for a dependency that's enforced in Kconfig, we
> have another problem to investigate.  Which I did, LMB is non-optional.

Please explain why is LMB non-optional ? I disagree. LMB for tools-only 
build is useless, hence it should not be enabled.

>>>>>> What kind of cleanup of lmb.h do you have in mind ?
>>>>>
>>>>> Remove it from include/image.h and fix any fall-out from that of files
>>>>> that got <lmb.h> indirectly when they needed it directly instead.
>>>>
>>>> Uh ... that is likely for a separate series, and a big one.
>>>
>>> Honestly, checking again, I'm not sure LMB=n is valid, ever.
>>
>> Why wouldn't it be ? For tools, LMB=n is perfectly valid.
> 
> Because it's never valid to disable LMB, LMB is what protects the
> running U-Boot.

We are talking about tools-only build here, not running U-Boot.

> It's nonsense to build u-boot on tools-only_defconfig but we don't have
> a way currently to remove "u-boot" from the all target.  Maybe once a
> few more of the hard/tricky CONFIG symbols get migrated to Kconfig we
> can then set tools-only_defconfig to NOT build u-boot at all.
> 
>>> That's how
>>> we keep our running U-Boot from being trivially overwritten and a huge
>>> number of security issues from being re-opened.
>>
>> Tools are not running U-Boot.
>>
>>> At this point, I think you should rework things to stop making
>>> CONFIG_LMB be optional, it should be a def_bool y.
>>
>> I disagree, see above.
> 
> The only reason "tools-only_defconfig" builds a useless u-boot binary
> today is in CI where it would be more work than it's worth to make CI
> exclude that from the build list.  But if you want to just do that
> instead, I'll also accept adding -x tools-only to the azure/gitlab jobs
> that build all other architectures, as tools-only is tested in its own
> build job, for it's only valid build target.

The tools-only build is also used elsewhere, to build just that, tools.

[...]