* [PATCH] efi/libstub: refactor cmd_stubcopy
@ 2019-02-12 3:44 Masahiro Yamada
2019-02-12 7:23 ` Ard Biesheuvel
0 siblings, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2019-02-12 3:44 UTC (permalink / raw)
To: Ard Biesheuvel, linux-kernel
Cc: Ingo Molnar, Masahiro Yamada, linux-efi, Kees Cook,
Alistair Strachan, Laura Abbott, Will Deacon, Ingo Molnar,
Nathan Chancellor
It took me a while to understand what is going on in the nested
if-blocks.
Simplify it by removing unneeded code.
- if_changed automatically adds 'set -e', so any failure in the
series of commands makes it immediately fail as a whole.
So, the outer if block is entirely redundant.
- Since commit 9c2af1c7377a ("kbuild: add .DELETE_ON_ERROR special
target"), GNU Make automatically deletes the target on any failure
in its recipe. The explicit 'rm -f $@' is redundant.
- surrounding commands with ( ) will spawn a subshell to execute them
in it, but it is rarely useful to do so.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
drivers/firmware/efi/libstub/Makefile | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index d984509..7788e8a 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -86,12 +86,13 @@ $(obj)/%.stub.o: $(obj)/%.o FORCE
# this time, use objcopy and leave all sections in place.
#
quiet_cmd_stubcopy = STUBCPY $@
- cmd_stubcopy = if $(STRIP) --strip-debug $(STUBCOPY_RM-y) -o $@ $<; \
- then if $(OBJDUMP) -r $@ | grep $(STUBCOPY_RELOC-y); \
- then (echo >&2 "$@: absolute symbol references not allowed in the EFI stub"; \
- rm -f $@; /bin/false); \
- else $(OBJCOPY) $(STUBCOPY_FLAGS-y) $< $@; fi \
- else /bin/false; fi
+ cmd_stubcopy = \
+ $(STRIP) --strip-debug $(STUBCOPY_RM-y) -o $@ $<; \
+ if $(OBJDUMP) -r $@ | grep $(STUBCOPY_RELOC-y); then \
+ echo "$@: absolute symbol references not allowed in the EFI stub" >&2; \
+ /bin/false; \
+ fi; \
+ $(OBJCOPY) $(STUBCOPY_FLAGS-y) $< $@
#
# ARM discards the .data section because it disallows r/w data in the
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] efi/libstub: refactor cmd_stubcopy
2019-02-12 3:44 [PATCH] efi/libstub: refactor cmd_stubcopy Masahiro Yamada
@ 2019-02-12 7:23 ` Ard Biesheuvel
2019-02-15 5:48 ` Masahiro Yamada
0 siblings, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2019-02-12 7:23 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Linux Kernel Mailing List, Ingo Molnar, linux-efi, Kees Cook,
Alistair Strachan, Laura Abbott, Will Deacon, Ingo Molnar,
Nathan Chancellor
On Tue, 12 Feb 2019 at 04:45, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> It took me a while to understand what is going on in the nested
> if-blocks.
>
> Simplify it by removing unneeded code.
>
> - if_changed automatically adds 'set -e', so any failure in the
> series of commands makes it immediately fail as a whole.
> So, the outer if block is entirely redundant.
>
> - Since commit 9c2af1c7377a ("kbuild: add .DELETE_ON_ERROR special
> target"), GNU Make automatically deletes the target on any failure
> in its recipe. The explicit 'rm -f $@' is redundant.
>
> - surrounding commands with ( ) will spawn a subshell to execute them
> in it, but it is rarely useful to do so.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Assuming that it still works as expected:
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
You can test this by adding a statically initialized global function
pointer to any of the libstub source files that get built for ARM.
Thanks!
> ---
>
> drivers/firmware/efi/libstub/Makefile | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index d984509..7788e8a 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -86,12 +86,13 @@ $(obj)/%.stub.o: $(obj)/%.o FORCE
> # this time, use objcopy and leave all sections in place.
> #
> quiet_cmd_stubcopy = STUBCPY $@
> - cmd_stubcopy = if $(STRIP) --strip-debug $(STUBCOPY_RM-y) -o $@ $<; \
> - then if $(OBJDUMP) -r $@ | grep $(STUBCOPY_RELOC-y); \
> - then (echo >&2 "$@: absolute symbol references not allowed in the EFI stub"; \
> - rm -f $@; /bin/false); \
> - else $(OBJCOPY) $(STUBCOPY_FLAGS-y) $< $@; fi \
> - else /bin/false; fi
> + cmd_stubcopy = \
> + $(STRIP) --strip-debug $(STUBCOPY_RM-y) -o $@ $<; \
> + if $(OBJDUMP) -r $@ | grep $(STUBCOPY_RELOC-y); then \
> + echo "$@: absolute symbol references not allowed in the EFI stub" >&2; \
> + /bin/false; \
> + fi; \
> + $(OBJCOPY) $(STUBCOPY_FLAGS-y) $< $@
>
> #
> # ARM discards the .data section because it disallows r/w data in the
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi/libstub: refactor cmd_stubcopy
2019-02-12 7:23 ` Ard Biesheuvel
@ 2019-02-15 5:48 ` Masahiro Yamada
2019-02-15 8:25 ` Ard Biesheuvel
0 siblings, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2019-02-15 5:48 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Linux Kernel Mailing List, Ingo Molnar, linux-efi, Kees Cook,
Alistair Strachan, Laura Abbott, Will Deacon, Ingo Molnar,
Nathan Chancellor
On Tue, Feb 12, 2019 at 4:26 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On Tue, 12 Feb 2019 at 04:45, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > It took me a while to understand what is going on in the nested
> > if-blocks.
> >
> > Simplify it by removing unneeded code.
> >
> > - if_changed automatically adds 'set -e', so any failure in the
> > series of commands makes it immediately fail as a whole.
> > So, the outer if block is entirely redundant.
> >
> > - Since commit 9c2af1c7377a ("kbuild: add .DELETE_ON_ERROR special
> > target"), GNU Make automatically deletes the target on any failure
> > in its recipe. The explicit 'rm -f $@' is redundant.
> >
> > - surrounding commands with ( ) will spawn a subshell to execute them
> > in it, but it is rarely useful to do so.
> >
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>
> Assuming that it still works as expected:
>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> You can test this by adding a statically initialized global function
> pointer to any of the libstub source files that get built for ARM.
>
> Thanks!
I tried that, and it failed as expected.
$ git diff
diff --git a/drivers/firmware/efi/libstub/arm32-stub.c
b/drivers/firmware/efi/libstub/arm32-stub.c
index becbda4..5ad7bbd 100644
--- a/drivers/firmware/efi/libstub/arm32-stub.c
+++ b/drivers/firmware/efi/libstub/arm32-stub.c
@@ -28,6 +28,8 @@ efi_status_t
check_platform_features(efi_system_table_t *sys_table_arg)
return EFI_SUCCESS;
}
+void * foo = (void *)check_platform_features;
+
static efi_guid_t screen_info_guid = LINUX_EFI_ARM_SCREEN_INFO_TABLE_GUID;
struct screen_info *alloc_screen_info(efi_system_table_t *sys_table_arg)
$ make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- defconfig
drivers/firmware/efi/libstub/
*** Default configuration is based on 'multi_v7_defconfig'
#
# configuration written to .config
#
scripts/kconfig/conf --syncconfig Kconfig
CALL scripts/checksyscalls.sh
CC drivers/firmware/efi/libstub/arm32-stub.o
STUBCPY drivers/firmware/efi/libstub/arm32-stub.stub.o
00000000 R_ARM_ABS32 check_platform_features
drivers/firmware/efi/libstub/arm32-stub.stub.o: absolute symbol
references not allowed in the EFI stub
drivers/firmware/efi/libstub/Makefile:80: recipe for target
'drivers/firmware/efi/libstub/arm32-stub.stub.o' failed
make[3]: *** [drivers/firmware/efi/libstub/arm32-stub.stub.o] Error 1
make[3]: *** Deleting file 'drivers/firmware/efi/libstub/arm32-stub.stub.o'
Makefile:1705: recipe for target 'drivers/firmware/efi/libstub/' failed
make[2]: *** [drivers/firmware/efi/libstub/] Error 2
/home/masahiro/workspace/bsp/linux/Makefile:300: recipe for target
'__build_one_by_one' failed
make[1]: *** [__build_one_by_one] Error 2
Makefile:160: recipe for target 'sub-make' failed
make: *** [sub-make] Error 2
--
Best Regards
Masahiro Yamada
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] efi/libstub: refactor cmd_stubcopy
2019-02-15 5:48 ` Masahiro Yamada
@ 2019-02-15 8:25 ` Ard Biesheuvel
2019-02-16 2:07 ` Masahiro Yamada
0 siblings, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2019-02-15 8:25 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Linux Kernel Mailing List, Ingo Molnar, linux-efi, Kees Cook,
Alistair Strachan, Laura Abbott, Will Deacon, Ingo Molnar,
Nathan Chancellor
On Fri, 15 Feb 2019 at 06:49, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Tue, Feb 12, 2019 at 4:26 PM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > On Tue, 12 Feb 2019 at 04:45, Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> > >
> > > It took me a while to understand what is going on in the nested
> > > if-blocks.
> > >
> > > Simplify it by removing unneeded code.
> > >
> > > - if_changed automatically adds 'set -e', so any failure in the
> > > series of commands makes it immediately fail as a whole.
> > > So, the outer if block is entirely redundant.
> > >
> > > - Since commit 9c2af1c7377a ("kbuild: add .DELETE_ON_ERROR special
> > > target"), GNU Make automatically deletes the target on any failure
> > > in its recipe. The explicit 'rm -f $@' is redundant.
> > >
> > > - surrounding commands with ( ) will spawn a subshell to execute them
> > > in it, but it is rarely useful to do so.
> > >
> > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >
> > Assuming that it still works as expected:
> >
> > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > You can test this by adding a statically initialized global function
> > pointer to any of the libstub source files that get built for ARM.
> >
> > Thanks!
>
>
> I tried that, and it failed as expected.
>
Great, thanks for double checking.
Are you taking this directly, or do you want me to take it via the EFI
tree? Either is fine with me
>
> $ git diff
> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c
> b/drivers/firmware/efi/libstub/arm32-stub.c
> index becbda4..5ad7bbd 100644
> --- a/drivers/firmware/efi/libstub/arm32-stub.c
> +++ b/drivers/firmware/efi/libstub/arm32-stub.c
> @@ -28,6 +28,8 @@ efi_status_t
> check_platform_features(efi_system_table_t *sys_table_arg)
> return EFI_SUCCESS;
> }
>
> +void * foo = (void *)check_platform_features;
> +
> static efi_guid_t screen_info_guid = LINUX_EFI_ARM_SCREEN_INFO_TABLE_GUID;
>
> struct screen_info *alloc_screen_info(efi_system_table_t *sys_table_arg)
> $ make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- defconfig
> drivers/firmware/efi/libstub/
> *** Default configuration is based on 'multi_v7_defconfig'
> #
> # configuration written to .config
> #
> scripts/kconfig/conf --syncconfig Kconfig
> CALL scripts/checksyscalls.sh
> CC drivers/firmware/efi/libstub/arm32-stub.o
> STUBCPY drivers/firmware/efi/libstub/arm32-stub.stub.o
> 00000000 R_ARM_ABS32 check_platform_features
> drivers/firmware/efi/libstub/arm32-stub.stub.o: absolute symbol
> references not allowed in the EFI stub
> drivers/firmware/efi/libstub/Makefile:80: recipe for target
> 'drivers/firmware/efi/libstub/arm32-stub.stub.o' failed
> make[3]: *** [drivers/firmware/efi/libstub/arm32-stub.stub.o] Error 1
> make[3]: *** Deleting file 'drivers/firmware/efi/libstub/arm32-stub.stub.o'
> Makefile:1705: recipe for target 'drivers/firmware/efi/libstub/' failed
> make[2]: *** [drivers/firmware/efi/libstub/] Error 2
> /home/masahiro/workspace/bsp/linux/Makefile:300: recipe for target
> '__build_one_by_one' failed
> make[1]: *** [__build_one_by_one] Error 2
> Makefile:160: recipe for target 'sub-make' failed
> make: *** [sub-make] Error 2
>
>
>
>
>
> --
> Best Regards
> Masahiro Yamada
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi/libstub: refactor cmd_stubcopy
2019-02-15 8:25 ` Ard Biesheuvel
@ 2019-02-16 2:07 ` Masahiro Yamada
2019-03-26 6:05 ` Masahiro Yamada
0 siblings, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2019-02-16 2:07 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Linux Kernel Mailing List, Ingo Molnar, linux-efi, Kees Cook,
Alistair Strachan, Laura Abbott, Will Deacon, Ingo Molnar,
Nathan Chancellor
On Sat, Feb 16, 2019 at 12:38 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 15 Feb 2019 at 06:49, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > On Tue, Feb 12, 2019 at 4:26 PM Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> > >
> > > On Tue, 12 Feb 2019 at 04:45, Masahiro Yamada
> > > <yamada.masahiro@socionext.com> wrote:
> > > >
> > > > It took me a while to understand what is going on in the nested
> > > > if-blocks.
> > > >
> > > > Simplify it by removing unneeded code.
> > > >
> > > > - if_changed automatically adds 'set -e', so any failure in the
> > > > series of commands makes it immediately fail as a whole.
> > > > So, the outer if block is entirely redundant.
> > > >
> > > > - Since commit 9c2af1c7377a ("kbuild: add .DELETE_ON_ERROR special
> > > > target"), GNU Make automatically deletes the target on any failure
> > > > in its recipe. The explicit 'rm -f $@' is redundant.
> > > >
> > > > - surrounding commands with ( ) will spawn a subshell to execute them
> > > > in it, but it is rarely useful to do so.
> > > >
> > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > >
> > > Assuming that it still works as expected:
> > >
> > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > >
> > > You can test this by adding a statically initialized global function
> > > pointer to any of the libstub source files that get built for ARM.
> > >
> > > Thanks!
> >
> >
> > I tried that, and it failed as expected.
> >
>
> Great, thanks for double checking.
>
> Are you taking this directly, or do you want me to take it via the EFI
> tree? Either is fine with me
>
>
Could you apply it to your EFI tree?
Thanks.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi/libstub: refactor cmd_stubcopy
2019-02-16 2:07 ` Masahiro Yamada
@ 2019-03-26 6:05 ` Masahiro Yamada
2019-03-26 7:54 ` Ard Biesheuvel
0 siblings, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2019-03-26 6:05 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Linux Kernel Mailing List, Ingo Molnar, linux-efi, Kees Cook,
Alistair Strachan, Laura Abbott, Will Deacon, Ingo Molnar,
Nathan Chancellor
Hi Ard,
On Sat, Feb 16, 2019 at 11:07 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Sat, Feb 16, 2019 at 12:38 AM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > On Fri, 15 Feb 2019 at 06:49, Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> > >
> > > On Tue, Feb 12, 2019 at 4:26 PM Ard Biesheuvel
> > > <ard.biesheuvel@linaro.org> wrote:
> > > >
> > > > On Tue, 12 Feb 2019 at 04:45, Masahiro Yamada
> > > > <yamada.masahiro@socionext.com> wrote:
> > > > >
> > > > > It took me a while to understand what is going on in the nested
> > > > > if-blocks.
> > > > >
> > > > > Simplify it by removing unneeded code.
> > > > >
> > > > > - if_changed automatically adds 'set -e', so any failure in the
> > > > > series of commands makes it immediately fail as a whole.
> > > > > So, the outer if block is entirely redundant.
> > > > >
> > > > > - Since commit 9c2af1c7377a ("kbuild: add .DELETE_ON_ERROR special
> > > > > target"), GNU Make automatically deletes the target on any failure
> > > > > in its recipe. The explicit 'rm -f $@' is redundant.
> > > > >
> > > > > - surrounding commands with ( ) will spawn a subshell to execute them
> > > > > in it, but it is rarely useful to do so.
> > > > >
> > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > > >
> > > > Assuming that it still works as expected:
> > > >
> > > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > >
> > > > You can test this by adding a statically initialized global function
> > > > pointer to any of the libstub source files that get built for ARM.
> > > >
> > > > Thanks!
> > >
> > >
> > > I tried that, and it failed as expected.
> > >
> >
> > Great, thanks for double checking.
> >
> > Are you taking this directly, or do you want me to take it via the EFI
> > tree? Either is fine with me
> >
> >
>
> Could you apply it to your EFI tree?
> Thanks.
Will you pick it up?
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi/libstub: refactor cmd_stubcopy
2019-03-26 6:05 ` Masahiro Yamada
@ 2019-03-26 7:54 ` Ard Biesheuvel
0 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2019-03-26 7:54 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Linux Kernel Mailing List, Ingo Molnar, linux-efi, Kees Cook,
Alistair Strachan, Laura Abbott, Will Deacon, Ingo Molnar,
Nathan Chancellor
On Tue, 26 Mar 2019 at 07:06, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> Hi Ard,
>
>
>
> On Sat, Feb 16, 2019 at 11:07 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > On Sat, Feb 16, 2019 at 12:38 AM Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> > >
> > > On Fri, 15 Feb 2019 at 06:49, Masahiro Yamada
> > > <yamada.masahiro@socionext.com> wrote:
> > > >
> > > > On Tue, Feb 12, 2019 at 4:26 PM Ard Biesheuvel
> > > > <ard.biesheuvel@linaro.org> wrote:
> > > > >
> > > > > On Tue, 12 Feb 2019 at 04:45, Masahiro Yamada
> > > > > <yamada.masahiro@socionext.com> wrote:
> > > > > >
> > > > > > It took me a while to understand what is going on in the nested
> > > > > > if-blocks.
> > > > > >
> > > > > > Simplify it by removing unneeded code.
> > > > > >
> > > > > > - if_changed automatically adds 'set -e', so any failure in the
> > > > > > series of commands makes it immediately fail as a whole.
> > > > > > So, the outer if block is entirely redundant.
> > > > > >
> > > > > > - Since commit 9c2af1c7377a ("kbuild: add .DELETE_ON_ERROR special
> > > > > > target"), GNU Make automatically deletes the target on any failure
> > > > > > in its recipe. The explicit 'rm -f $@' is redundant.
> > > > > >
> > > > > > - surrounding commands with ( ) will spawn a subshell to execute them
> > > > > > in it, but it is rarely useful to do so.
> > > > > >
> > > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > > > >
> > > > > Assuming that it still works as expected:
> > > > >
> > > > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > >
> > > > > You can test this by adding a statically initialized global function
> > > > > pointer to any of the libstub source files that get built for ARM.
> > > > >
> > > > > Thanks!
> > > >
> > > >
> > > > I tried that, and it failed as expected.
> > > >
> > >
> > > Great, thanks for double checking.
> > >
> > > Are you taking this directly, or do you want me to take it via the EFI
> > > tree? Either is fine with me
> > >
> > >
> >
> > Could you apply it to your EFI tree?
> > Thanks.
>
>
>
> Will you pick it up?
>
Yes, it's on my list for v5.2
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-03-26 7:54 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-12 3:44 [PATCH] efi/libstub: refactor cmd_stubcopy Masahiro Yamada
2019-02-12 7:23 ` Ard Biesheuvel
2019-02-15 5:48 ` Masahiro Yamada
2019-02-15 8:25 ` Ard Biesheuvel
2019-02-16 2:07 ` Masahiro Yamada
2019-03-26 6:05 ` Masahiro Yamada
2019-03-26 7:54 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).