Hi Tom, On Sun, 22 Oct 2023 at 16:45, Tom Rini <tr...@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 <tr...@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.aka...@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.g...@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 <s...@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 > > > 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. Re #ifdef, you will still see code discarded with sandbox if 'if IS_ENABLED()' is used. It's just that the link-time Sandbox doesn't really have a need to discard unused code. It doesn't make much difference to the binary size, from what I can see, partly because we enable as much as we can. No LTO: before: 3652159 220840 126576 3999575 3d0757 /tmp/b/sandbox/u-boot after: 3515872 204016 124528 3844416 3aa940 /tmp/b/sandbox/u-boot Saving 3.7% With LTO: before: 3391148 212224 124704 3728076 38e2cc /tmp/b/sandbox/u-boot after: 3356324 200664 124704 3681692 382d9c /tmp/b/sandbox/u-boot Saving 1% That reflects the fact that LTO is quite a bit more powerful than linker hacks. The other thing I have noticed over the years is that linker discarding can change behaviour when unrelated changes are made. For example, if I refactor a function the linker decides that a function it calls is now needed, whereas before it was not. That sort of thing is annoying with sandbox. > > > At least for me 'u-boot -D' doesn't work with Heinrich's patch. Do we > > really need to make this change? It will rapidly devolve to the point > > that coming back would be extremely painful. > > Do we absolutely need which, gc-sections or LTO? Yes, we do. And I > don't see immediately how that changes --default_fdt ? I hope that with all the clang effort we might be closer to having sandbox behave well with this change. But I still don't see the point. Regards, SImon
