On Sat, Sep 04, 2021 at 04:03:25PM +0200, Marek Vasut wrote:
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.
I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of: 1. Nothing else. 2. Add tools-only to the exclude list in the "build everything else" CI job. 3. Make CONFIG_LMB be def_bool y.