[PATCH v3 22/32] efi: Update EFI_LOADER to depend on DM_ETH

[Tom Rini]

On Mon, Oct 23, 2023 at 10:13:54AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 23 Oct 2023 at 06:16, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Mon, Oct 23, 2023 at 12:05:28AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Sun, 22 Oct 2023 at 16:45, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Sun, Oct 22, 2023 at 02:55:32PM -0700, Simon Glass wrote:
> > > > > Hi,
> > > > >
> > > > > On Sun, 22 Oct 2023 at 07:59, Tom Rini <trini at konsulko.com> wrote:
> > > > > >
> > > > > > On Sun, Oct 22, 2023 at 10:29:22AM -0400, Tom Rini wrote:
> > > > > > > On Sun, Oct 22, 2023 at 08:08:11AM +0200, Heinrich Schuchardt wrote:
> > > > > > > > On 10/21/23 20:26, Tom Rini wrote:
> > > > > > > > > On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote:
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro
> > > > > > > > > > <takahiro.akashi at linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote:
> > > > > > > > > > > > Hi Heinrich,
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > On 10/17/23 16:09, Tom Rini wrote:
> > > > > > > > > > > > > > On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is
> > > > > > > > > > > > > > > available, add it as an explicit dependency.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > (no changes since v2)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Changes in v2:
> > > > > > > > > > > > > > > - Add new patch to update EFI_LOADER to depend on DM_ETH
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >    lib/efi_loader/Kconfig | 1 +
> > > > > > > > > > > > > > >    1 file changed, 1 insertion(+)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > > > > > > > > > > > index 13cad6342c36..fca4b3eef270 100644
> > > > > > > > > > > > > > > --- a/lib/efi_loader/Kconfig
> > > > > > > > > > > > > > > +++ b/lib/efi_loader/Kconfig
> > > > > > > > > > > > > > > @@ -11,6 +11,7 @@ config EFI_LOADER
> > > > > > > > > > > > > > >       # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB
> > > > > > > > > > > > > > >       depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT
> > > > > > > > > > > > > > >       depends on BLK
> > > > > > > > > > > > > > > +    depends on DM_ETH
> > > > > > > > > > > > > > >       depends on !EFI_APP
> > > > > > > > > > > > > > >       default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8
> > > > > > > > > > > > > > >       select CHARSET
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Does this work for you Heinrich, or do you want to clarify the
> > > > > > > > > > > > > > dependencies (and re-organize the code as needed) around networking?
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > We should be able to boot via EFI on devices without U-Boot network support.
> > > > > > > > > > > > >
> > > > > > > > > > > > > We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking
> > > > > > > > > > > > > eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects
> > > > > > > > > > > > > CONFIG_DM_ETH.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Why is this not sufficient?
> > > > > > > > > > > > > Is there a configuration that does not build?
> > > > > > > > > > > >
> > > > > > > > > > > > The point of this series is to disable CMDLINE and fix up what breaks.
> > > > > > > > > > > >
> > > > > > > > > > > > In this case we have some sort of breakage...perhaps Tom has already
> > > > > > > > > > > > found it, but otherwise could you take a look?
> > > > > > > > > > > >
> > > > > > > > > > > > We should be able to disable NET and LTO in sandbox and still build.
> > > > > > > > > > > > But this fails at present[1]. You can try it on -master
> > > > > > > > > > >
> > > > > > > > > > > Obviously, it would be necessary to enclose efi_dp_from_eth()
> > > > > > > > > > > with "if defined(CONFIG_NETDEVICES)" (or DM_ETH).
> > > > > > > > > > > Then, we could drop "depends on DM_ETH".
> > > > > > > > > >
> > > > > > > > > > Strange that it only happens on the non-LTO board, though?
> > > > > > > > >
> > > > > > > > > There's two issues. The first of which is that I think you need to
> > > > > > > > > re-check your error exactly? With my series, and LTO also disabled the
> > > > > > > > > problem is a call to efi_get_image_parameters() as that's defined in
> > > > > > > > > cmd/bootefi.c, but also only used with cmdline invocations. So we can
> > > > > > > > > fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE)
> > > > > > > > > around that, and then discard efi_dp_from_name() entirely.
> > > > > > > > >
> > > > > > > > > The second issue is that with LTO we more completely find the cases
> > > > > > > > > where if x() calls y() and y() is undefined but nothing calls x() we can
> > > > > > > > > just discard x() and not care that y() is undefined.
> > > > > > > > >
> > > > > > > >
> > > > > > > > I will send a patch for function efi_dp_from_eth().
> > > > > > >
> > > > > > > There's no problem with efi_dp_from_eth as far as I can tell.
> > > > > > >
> > > > > > > > @Simon
> > > > > > > >
> > > > > > > > One thing that I don't understand is why we don't let the linker
> > > > > > > > eliminate the unused functions on the sandbox.
> > > > > > > >
> > > > > > > > On other architectures we put each function into a separate text section
> > > > > > > > and let the linker eliminate the unused text sections:
> > > > > > > >
> > > > > > > > arch/riscv/config.mk:29:
> > > > > > > > PLATFORM_RELFLAGS       += -fno-common -ffunction-sections -fdata-sections
> > > > > > > > LDFLAGS_u-boot          += --gc-sections -static -pie
> > > > > > > >
> > > > > > > > Shouldn't we keep the sandbox close to what other architectures do?
> > > > > > >
> > > > > > > Oh my, I didn't realize that sandbox was missing the garbage collection
> > > > > > > stuff.  Yes, that needs to be fixed first, then we can see what's next
> > > > > > > to change, as there are some issues (my series first fixed CMDLINE=n on
> > > > > > > qemu_arm64).
> > > > > >
> > > > > > My super quick pass at enabling
> > > > > > function-sections/data-sections/gc-sections shows there's nothing
> > > > > > further needed for CMDLINE=n and LTO=n on sandbox, not even the part I
> > > > > > was looking at before.
> > > > >
> > > > > I am not sure about the original reason, but sandbox probably
> > > > > pre-dates wholesale enabling of gc-sections (I haven't looked). There
> > > > > is also the issue of whether the host toolchain supports it. Sandbox
> > > > > is supposed to run on a variety of OS types.
> > > >
> > > > It doesn't predate gc-sections, we've just never moved it to the
> > > > top-level Makefiles (and disabled on LTO) like we should have a long
> > > > time ago.  It's supported by all the compilers we support, and it's a
> > > > required feature (or, LTO) for U-Boot.
> > >
> > > Remember that the sandbox linker script is special in that it adds to
> > > the existing default one provided by the toolchain
> >
> > Yes, and we've also been using gc-sections since 2008, from a quick look
> > at the logs.
> 
> Right, I just mean that sandbox does not use a standalone linker
> script.

Yes it does, arch/sandbox/cpu/u-boot.lds.

> It is not common to garbage-collect sections in executables,
> so far as I know.

I'm not sure how that's relevant here, honestly. And we didn't ask the
toolchain community to invent this for us, it's a standard feature that
we've found helpful for a number of reasons.

> > > > > I am also not sure of the point ot it...since it is normally pretty
> > > > > easy to use Kconfig to control features.
> > > >
> > > > That's not the point of the feature.  The point is that we can discard
> > > > unused code without having to rely on #ifdef and __maybe_unused.
> > >
> > > That doesn't match with my understanding. An unused static will still
> > > produce a warning if it is unused.
> >
> > Yes, you're misunderstanding how this feature works and why it's
> > required.  With v4 of my series, on sandbox and CMDLINE=n and LTO=n:
> > /usr/bin/ld: boot/scene_textline.o: in function `scene_textline_send_key':
> > /home/trini/work/u-boot/u-boot/boot/scene_textline.c:157: undefined reference to `cread_line_process_ch'
> > /usr/bin/ld: boot/scene_textline.o: in function `scene_textline_open':
> > /home/trini/work/u-boot/u-boot/boot/scene_textline.c:222: undefined reference to `cli_cread_init'
> > /usr/bin/ld: common/bootstage.o: in function `bootstage_fdt_add_report':
> > /home/trini/work/u-boot/u-boot/common/bootstage.c:323: undefined reference to `working_fdt'
> > /usr/bin/ld: common/cli.o: in function `run_commandf':
> > /home/trini/work/u-boot/u-boot/common/cli.c:154: undefined reference to `run_command'
> > /usr/bin/ld: drivers/fastboot/fb_common.o: in function `fastboot_boot':
> > /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_common.c:137: undefined reference to `run_command'
> > /usr/bin/ld: drivers/fastboot/fb_command.o: in function `fastboot_acmd_complete':
> > /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_command.c:350: undefined reference to `run_command'
> > /usr/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_name':
> > /home/trini/work/u-boot/u-boot/lib/efi_loader/efi_device_path.c:1095: undefined reference to `efi_get_image_parameters'
> > /usr/bin/ld: net/net.o: in function `net_auto_load':
> > /home/trini/work/u-boot/u-boot/net/net.c:349: undefined reference to `tftp_start'
> > collect2: error: ld returned 1 exit status
> >
> > And with re-enabling gc-sections:
> > /usr/bin/ld: drivers/fastboot/fb_common.o: in function `fastboot_boot':
> > /home/trini/work/u-boot/u-boot/drivers/fastboot/fb_common.c:137: undefined reference to `run_command'
> > collect2: error: ld returned 1 exit status
> >
> > Because all of those other link failures are in unreachable code and the
> > rest of the code base is already setup to know that x() can call y() and
> > y() can just be undefined at link time if x() is never called and ends
> > up discarded.
> 
> All of those problems were corrected by my series. Perhaps that
> accounts for the difference between them?

All of these are not real problems at this point in time is my point.

> Just to take one example, working_fdt, while it might seem convenient
> to leave it in cmd/fdt.c it doesn't actually make sense, since the var
> is set when booting. Without that var DT fixups can't happen. So why
> not pick up my patch that moves it to boot/fdt_support.c where it
> belongs?

And the issue is that when the command line is disabled NOTHING uses
"working_fdt". We don't need it. It would be discarded by real
platforms, if we moved it.

To rephrase things another way, skimming the code, that
bootm_find_images() does:
	if (IS_ENABLED(CONFIG_CMD_FDT))
		set_working_fdt_addr(map_to_sysmem(images.ft_addr));

And so we never set "working_fdt" when CMDLINE is off (since CMD_FDT is
off) IS the bug, and that needs to be fixed. That's when you should
figure out where "working_fdt" should be (and also maybe re-name it or
the local variable named "working_fdt" that the pxe_utils.c code uses)
and not before then.

> Looking at the list of errors, we should just fix them. They all
> indicate real problems with the structure of the code.

No, they do not. The codebase assume that we discard unreachable
functions. This allows us to NOT have to use #ifdefs and also not have
to make otherwise unreasonable code splits.

> Another example
> is fastboot which needs a condition for CMDLINE.

This is an example where yes, the functionality requires CMDLINE.
Because reading the code shows that fastboot will literally run
"bootcmd", and so requires that to be available.

> > I honestly didn't realize sandbox hadn't had this critical feature
> > enabled and I hope we haven't been doing too many odd shuffles of the
> > codebase because of it.
> 
> I'm a bit unsure where you are heading with this. Ultimate we don't
> want to disable all the functionality when CMDLINE is disabled. I
> think with the addition of 5-6 patches in your series you can fix it.

Where I'm heading with this is to say that sandbox must work like a real
platform does and discard unreachable code. You don't know what code
is or is not really needed for functionality X and needs to be moved for
it to not rely on what's in the command file vs elsewhere until you act
like other platforms do.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231023/0b28863a/attachment.sig>