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. -- Tom
signature.asc
Description: PGP signature