Received-SPF: pass (google.com: domain of linux-kernel+bounces-4447-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20231218080127.907460-1-masahiroy@kernel.org>
In-Reply-To: <20231218080127.907460-1-masahiroy@kernel.org>
From: Ard Biesheuvel <ardb@kernel.org>
Date: Mon, 18 Dec 2023 23:26:58 +0100
Message-ID: <CAMj1kXFmoXKLzFQnfTgatSpsQj=6HnExRoYtH_jaYakGZFwrkg@mail.gmail.com>
Subject: Re: [PATCH] efi/libstub: zboot: do not use $(shell ...) in cmd_copy_and_pad
To: Masahiro Yamada <masahiroy@kernel.org>
Cc: linux-efi@vger.kernel.org, linux-kbuild@vger.kernel.org, 
	Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, 
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Mon, 18 Dec 2023 at 09:01, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> You do not need to use $(shell ...) in recipe lines, as they are already
> executed in a shell. An alternative solution is $$(...), which is an
> escaped sequence of the shell's command substituion, $(...).
>
> For this case, there is a reason to avoid $(shell ...).
>
> Kbuild detects command changes by using the if_changed macro, which
> compares the previous command recorded in .*.cmd with the current
> command from Makefile. If they differ, Kbuild re-runs the build rule.
>
> To diff the commands, Make must expand $(shell ...) first. It means that
> hexdump is executed every time, even when nothing needs rebuilding. If
> Kbuild determines that vmlinux.bin needs rebuilding, hexdump will be
> executed again to evaluate the 'cmd' macro, one more time to really
> build vmlinux.bin, and finally yet again to record the expanded command
> into .*.cmd.
>
> Replace $(shell ...) with $$(...) to avoid multiple, unnecessay shell
> evaluations. Since Make is agnostic about the shell code, $(...), the
> if_changed macro compares the string "$(hexdump -s16 -n4 ...)" verbatim,
> so hexdump is run only for building vmlinux.bin.
>
> For the same reason, $(shell ...) in EFI_ZBOOT_OBJCOPY_FLAGS should be
> eliminated.
>
> While I was here, I replaced '&&' with ';' because a command for
> if_changed is executed with 'set -e'.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>
>  arch/arm64/boot/Makefile                    | 2 +-
>  drivers/firmware/efi/libstub/Makefile.zboot | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/Makefile b/arch/arm64/boot/Makefile
> index 1761f5972443..a5a787371117 100644
> --- a/arch/arm64/boot/Makefile
> +++ b/arch/arm64/boot/Makefile
> @@ -44,7 +44,7 @@ EFI_ZBOOT_BFD_TARGET  := elf64-littleaarch64
>  EFI_ZBOOT_MACH_TYPE    := ARM64
>  EFI_ZBOOT_FORWARD_CFI  := $(CONFIG_ARM64_BTI_KERNEL)
>
> -EFI_ZBOOT_OBJCOPY_FLAGS        = --add-symbol zboot_code_size=0x$(shell \
> +EFI_ZBOOT_OBJCOPY_FLAGS        = --add-symbol zboot_code_size=0x$$( \
>                                 $(NM) vmlinux|grep _kernel_codesize|cut -d' ' -f1)
>
>  include $(srctree)/drivers/firmware/efi/libstub/Makefile.zboot
> diff --git a/drivers/firmware/efi/libstub/Makefile.zboot b/drivers/firmware/efi/libstub/Makefile.zboot
> index 2c489627a807..65ffd0b760b2 100644
> --- a/drivers/firmware/efi/libstub/Makefile.zboot
> +++ b/drivers/firmware/efi/libstub/Makefile.zboot
> @@ -5,8 +5,8 @@
>  # EFI_ZBOOT_FORWARD_CFI
>
>  quiet_cmd_copy_and_pad = PAD     $@
> -      cmd_copy_and_pad = cp $< $@ && \
> -                        truncate -s $(shell hexdump -s16 -n4 -e '"%u"' $<) $@
> +      cmd_copy_and_pad = cp $< $@; \
> +                        truncate -s $$(hexdump -s16 -n4 -e '"%u"' $<) $@
>
>  # Pad the file to the size of the uncompressed image in memory, including BSS
>  $(obj)/vmlinux.bin: $(obj)/$(EFI_ZBOOT_PAYLOAD) FORCE
> --
> 2.40.1
>
>