Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

From: Simon Glass <sjg@chromium.org>
To: AKASHI Takahiro <takahiro.akashi@linaro.org>,
	Simon Glass <sjg@chromium.org>,
	Heinrich Schuchardt <xypron.glpk@gmx.de>,
	Masami Hiramatsu <masami.hiramatsu@linaro.org>,
	 U-Boot Mailing List <u-boot@lists.denx.de>,
	Lukasz Majewski <lukma@denx.de>, Peng Fan <peng.fan@nxp.com>,
	 Bin Meng <bmeng.cn@gmail.com>,
	Jaehoon Chung <jh80.chung@samsung.com>, Stefan Roese <sr@denx.de>,
	Ilias Apalodimas <ilias.apalodimas@linaro.org>
Subject: Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model
Date: Tue, 8 Mar 2022 20:10:01 -0700	[thread overview]
Message-ID: <CAPnjgZ3uCUroMNjuZQdpcOMgmpCDYVGYnqMFiq4f-df2aneMzQ@mail.gmail.com> (raw)
In-Reply-To: <20220309024839.GC136899@laputa>

Hi Takahiro,

On Tue, 8 Mar 2022 at 19:48, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> Hi Simon,
>
> On Tue, Mar 08, 2022 at 07:34:15PM -0700, Simon Glass wrote:
> > Hi Takahiro,
> >
> > On Tue, 8 Mar 2022 at 19:10, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > Heinrich, Simon,
> > >
> > > On Tue, Mar 08, 2022 at 05:49:13PM +0100, Heinrich Schuchardt wrote:
> > > > On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > > > With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > > > disk objects dynamically at runtime based on block device's probing.
> > > > > (See "issues" below.)
> > > > >
> > > > > [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > > >
> > > > This series together with Simon's series breaks multiple boards due to
> > > > size constraints:
> > > >
> > > > https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > > >
> > > > Please, investigate how to work around this issue.
> > >
> > > I have already mentioned this size issue in my cover-letter
> > > in order to let reviewers aware of it and discuss a possible solution:
> > >
> > > ===8<===
> > > Issues:
> > > =======
> > > * The image size of U-Boot may increase. CI build test complains,
> > >   for instance,
> > >     rcar3_salvator-x:
> > >       "u-boot.img exceeds file size limit: ... excess: 0x79c bytes"
> > >     phycore-rk3288:
> > >       "SPL image is too large (size 0x8800 than 0x8000)"
> > >
> > >   See [2].
> > >
> > > [2] https://dev.azure.com/u-boot/u-boot/_build/results?buildId=3770&view=results
> > > ===>8===
> > >
> > > I have dug into rcar3_salvator-x case; I removed *all* the commits
> > > in this series and yet enabled CONFIG_EVENT, CONFIG_EVENT_DYNAMIC
> > > and CONFIG_DM_EVENT, which are all required for enabling my patch,
> > > with Simon's patch applied on top of v2022.04-rc3.
> > >
> > > Then I still see this size problem:
> > > ===8<===
> > >   ...
> > >   MKIMAGE u-boot.img
> > > u-boot.img exceeds file size limit:
> > >   limit:  0x100000 bytes
> > >   actual: 0x100036 bytes
> > >   excess: 0x36 bytes
> > > ===>8===
> > >
> > > So I have no way to deal with it.
> > >
> > > FYI, the combination of EVENT, EVENT_DYNAMIC and DM_EVENT will
> > > increase the binary size by up to 0x1b2 for rcar3_salvator-x and
> > > it seems the binary has almost already reached its maximum size
> > > even now.
> >
> > So you do need EVENT_DYNAMIC?
>
> Unfortunately, yes.
> When I rebased my patch set to your v2, I tried to use *static*
> bindings, but some of ut tests, including dm_test_blk_base and
> dm_test_blk_usb, failed.

OK. Well maybe there is a filesystem in there that is not needed? 1MB
is a huge size! Can we disable EFI_LOADER on that board?

>
> This can happen because, with static bindings, efi's callback function
> (efi_disk_probe) is unconditionally called even when UEFI subsystem has
> not been initialized yet.

Yes, I have seen things like that too.

>
> -Takahiro Akashi
>
> > Does it make sense to make enabling the partition support an option,
> > instead of mandatory?

What about this one? ^^

Regards,
Simon