[PATCH v2 7/9] Make EFI_LOADER depend on DM and OF_CONTROL

Mon Mar 28 08:35:09 CEST 2022

Hi Tom,

On Fri, 30 Jul 2021 at 16:08, Tom Rini <trini at konsulko.com> wrote:
>
> On Fri, Jul 30, 2021 at 03:48:15PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 30 Jul 2021 at 15:33, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Fri, Jul 30, 2021 at 01:02:18PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 29 Jul 2021 at 07:52, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Wed, Jul 28, 2021 at 07:44:37PM -0600, Simon Glass wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Wed, 28 Jul 2021 at 17:55, Tom Rini <trini at konsulko.com> wrote:
> > > > > > >
> > > > > > > On Thu, Jul 29, 2021 at 01:45:49AM +0200, Heinrich Schuchardt wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > On 7/27/21 12:07 AM, Tom Rini wrote:
> > > > > > > > > On Fri, Jul 02, 2021 at 12:36:18PM -0600, Simon Glass wrote:
> > > > > > > > >
> > > > > > > > > > This feature should never have been made available when driver model
> > > > > > > > > > or devicetree are disabled. Add these as conditions, so that we don't
> > > > > > > > > > create even more barriers to migration.
> > > > > > > > > >
> > > > > > > > > > Add a note about the substantial size increment associated with this
> > > > > > > > > > option.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > Changes in v2:
> > > > > > > > > > - Split out new patch to make EFI_LOADER depend on DM and OF_CONTROL
> > > > > > > > > > - Note the approximate size of this feature in the help
> > > > > > > > > >
> > > > > > > > > >   lib/efi_loader/Kconfig | 4 +++-
> > > > > > > > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > > > > > > index 6242caceb7f..466abfed300 100644
> > > > > > > > > > --- a/lib/efi_loader/Kconfig
> > > > > > > > > > +++ b/lib/efi_loader/Kconfig
> > > > > > > > > > @@ -1,6 +1,6 @@
> > > > > > > > > >   config EFI_LOADER
> > > > > > > > > >           bool "Support running UEFI applications"
> > > > > > > > > > - depends on OF_LIBFDT && ( \
> > > > > > > > > > + depends on OF_LIBFDT && DM && OF_CONTROL && ( \
> > > > > > > >
> > > > > > > > Didn't Tom eliminate all boards without CONFIG_DM? Shouldn't we get rid
> > > > > > > > of this symbol?
> > > > > > >
> > > > > > > No, but I did send out a message about that today as we're very close.
> > > > > > > Much closer than I had expected us to be.
> > > > > >
> > > > > > Note we will still have SPL_DM, I think.
> > > > >
> > > > > Right.
> > > > >
> > > > > > > > Are there boards using DM and not OF_CONTROL or OF_CONTROL and not DM?
> > > > > > >
> > > > > > > Yes, a few.  It's vexpress_aemv8a_semi, warp (fixed by
> > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20210402180552.1075997-2-pbrobinson@gmail.com/
> > > > > > > so false positive), cm_t335, devkit8000, armadillo-800eva, kzm9g and sniper.
> > > > > > >
> > > > > > > > Why are these separate symbols? Isn't this symbol to be eliminated, too?
> > > > > > >
> > > > > > > Simon?
> > > > > >
> > > > > > If we do eliminate DM it will be in a separate series. Like Tom I am
> > > > > > pleasantly surprised at how close we are.
> > > > > >
> > > > > > But please let's consider patches on their merits. It is fine to reply
> > > > > > with a wishlist but even better to reply with a follow-up patch.
> > > > >
> > > > > OK.  So, build-wise, EFI_LOADER does not require OF_CONTROL.
> > > >
> > > > I strongly believe it should (and it should have 5 years ago too). New
> > > > features should not be built on pre-migration stuff.
> > >
> > > Well, what legacy interfaces is it using?  That's something to figure
> > > out and address as needed.
> >
> > It was built on non-DM and I believe it still has code for non-DM
> > (e.g. look for DM_MMC). Without DM, OF_CONTROL has no purpose IMO.
> >
> > Perhaps Heinrich has cleaned a lot of that old cruft out now?
>
> Now that DM_MMC and DM_VIDEO are mandatory, there is some cruft that can
> be removed, both there and probably tree-wide.  But that's not the same
> as OF_CONTROL.  Not all DM_xxx requires OF_CONTROL support.
>
> > > > > > Somewhat related, I think we need to create a separate symbol which
> > > > > > means (OF_CONTROL && !OF_PLATDATA) so we can just check one thing.
> > > > > >
> > > > > > Also I think we should push of-platdata, since otherwise we're going
> > > > > > to hit the same problem of migrating SPL boards to DM one day.
> > > > >
> > > > > Note that we don't have CONFIG_OF_PLATDATA just
> > > > > CONFIG_(SPL|TPL)_OF_PLATDATA.
> > > >
> > > > Indeed. But we haven't defined it because we don't want to permit it.
> > > > It is just for constrained environments and we assume that U-Boot
> > > > proper has enough space (how else could it load Linux?)
> > >
> > > If OF_PLATDATA for U-Boot itself makes sense or not is a separate
> > > discussion to have.
> >
> > OK, I'd love to hear the reasoning on that one day.
>
> This might come up again real soon now in the context of nokia_rx51 and
> migrating away from the very old MUSB gadget driver and to modern
> DM_USB_GADGET.  The platform is DM=y and OF_CONTROL=n and hard space
> constrained.

Oh I see. Well I don't believe it would be that hard to enable it, but
if it is just for one platform, is it worth it?

>
> > > > > > > > lib/efi_loader/efi_disk.c is the only place where we maintain duplicate
> > > > > > > > code for DM and non-DM. A dependency on CONFIG_BLK (which itself depends
> > > > > > > > on CONFIG_DM) would make more sense to me. But only in a patch
> > > > > > > > eliminating the non-BLK code.
> > > > > > >
> > > > > > > I just know that off-hand, partition + disk + block has some corner
> > > > > > > case, but maybe that corner case is unintentional in terms of usage
> > > > > > > today.
> > > > > > >
> > > > > > > > > >                   ARM && (SYS_CPU = arm1136 || \
> > > > > > > > > >                           SYS_CPU = arm1176 || \
> > > > > > > > > >                           SYS_CPU = armv7   || \
> > > > > > > > > > @@ -25,6 +25,8 @@ config EFI_LOADER
> > > > > > > > > >             will expose the UEFI API to a loaded application, enabling it to
> > > > > > > > > >             reuse U-Boot's device drivers.
> > > > > > > > > >
> > > > > > > > > > +   For ARM 32-bit, this adds about 90KB to the size of U-Boot.
> > > > > > > > > > +
> > > > > > > >
> > > > > > > > There is no unit ISO prefix K. Do you mean KiB?
> > > > > > > >
> > > > > > > > 90 KiB may be the value today. Will you update it regularly? Otherwise
> > > > > > > > don't put a number here.
> > > > > > > >
> > > > > > > > I can't see that the effect on size is truly architecture specific. Why
> > > > > > > > do you refer to 32bit ARM?
> > > > > > > >
> > > > > > > > Such a comment would better fit into a documentation chapter on
> > > > > > > > downsizing U-Boot.
> > > > > > >
> > > > > > > Yes, we should probably drop that specific note.
> > > > > >
> > > > > > From my POV I really like these notes in Kconfig. They appear in a few
> > > > > > places and provide people with rough guidance. I'd like to see more of
> > > > > > them. I don't know how we can keep them up-to-date, although I'd argue
> > > > > > that they should stay constant, if we are holding to our no-bloat
> > > > > > ideal.
> > > > >
> > > > > I feel like EFI gets a bit of an undeserved reputation here.  It's not
> > > > > growing any worse than the rest of the world is over fixes and error
> > > > > correction (which is to say, 16/32/40 bytes here and there).  And
> > > > > there's not "big" new default features coming in.
> > > >
> > > > We can agree on the 'reputation' bit but I can't think of a more
> > > > deserving feature :-)
> > > >
> > > > I keep getting the capsule-update series in my inbox, for example and
> > > > I know there is TPM stuff in the works.
> > >
> > > Yes, but TPM and capsule-update won't be default enabled for all
> > > platforms.  I run every PR I get (and branch I make and merge) through a
> > > before/after world build and use buildman's sizes tools to check (aside,
> > > I'd love some csv output format for that, but I haven't gotten around to
> > > thinking on how to do that) and that's where I'm coming from on this.  I
> > > am keeping an eye out for default new features everywhere, and default
> > > new functionality everywhere.
> >
> > We could have buildman output some stats file as 'artifacts' in gitlab
> > perhaps, then have a script that picks them up and stores them
> > somewhere for analysis?
>
> My first thought would just be to somehow put the output of --show-sizes
> as a CSV so I could then figure out a way to filter out that I've seen
> for example simple_strtoul changed size on every platform.  What else is
> there?  Today I page through the output and am good, but not perfect, at
> spotting the outliers (for example, I had to tweak "Makefile: Move phy
> rules into drivers/phy" because a few platforms did not enable
> CONFIG_PHY before and those stood out from the rest of the size changes
> due to different optimization due to changed link order).  Tooling
> around making that less error prone would be good.  Perhaps outputting
> all functions and sizes and platform and full/spl/tpl to a csv or json
> would be the first step to being able to compare builds.

Sounds like something that could be added to buildman, yes. I'm not
sure if there are other people willing to take this on?

Regards,
Simon