efi/libstub: refactor cmd_stubcopy

Message ID 1549943095-10956-1-git-send-email-yamada.masahiro@socionext.com
State New
Headers show
Series
  • efi/libstub: refactor cmd_stubcopy
Related show

Commit Message

Masahiro Yamada Feb. 12, 2019, 3:44 a.m.
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

Comments

Ard Biesheuvel Feb. 12, 2019, 7:23 a.m. | #1
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

>
Masahiro Yamada Feb. 15, 2019, 5:48 a.m. | #2
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
Ard Biesheuvel Feb. 15, 2019, 8:25 a.m. | #3
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
Masahiro Yamada March 26, 2019, 6:05 a.m. | #4
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
Ard Biesheuvel March 26, 2019, 7:54 a.m. | #5
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

Patch

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