| Message ID | 1549943095-10956-1-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Accepted |
| Commit | e8d368ad20f514dce86a64931fe4a6f06a0a6703 |
| Headers | show |
| Series | efi/libstub: refactor cmd_stubcopy | expand |
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 >
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
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
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
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
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
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(-) -- 2.7.4