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

[Tom Rini]

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.

-- 
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/20231022/fad8b0b6/attachment.sig>