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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5990 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]