diff mbox series

[v9,03/11] tools: build mkeficapsule with tools-only_defconfig

Message ID 20220118043954.55940-4-takahiro.akashi@linaro.org
State Superseded
Headers show
Series efi_loader: capsule: improve capsule authentication support | expand

Commit Message

AKASHI Takahiro Jan. 18, 2022, 4:39 a.m. UTC 
Add CONFIG_TOOLS_MKEFICAPSULE. Then we want to always build mkeficapsule
if tools-only_defconfig is used.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 configs/tools-only_defconfig | 1 +
 tools/Kconfig                | 8 ++++++++
 tools/Makefile               | 3 +--
 3 files changed, 10 insertions(+), 2 deletions(-)

Comments

Heinrich Schuchardt Jan. 19, 2022, 4:08 p.m. UTC | #1 
On 1/18/22 05:39, AKASHI Takahiro wrote:
> Add CONFIG_TOOLS_MKEFICAPSULE. Then we want to always build mkeficapsule
> if tools-only_defconfig is used.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>   configs/tools-only_defconfig | 1 +
>   tools/Kconfig                | 8 ++++++++
>   tools/Makefile               | 3 +--
>   3 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig
> index f482c9a1c1b0..5427797dd4c3 100644
> --- a/configs/tools-only_defconfig
> +++ b/configs/tools-only_defconfig
> @@ -31,3 +31,4 @@ CONFIG_I2C_EDID=y
>   # CONFIG_VIRTIO_MMIO is not set
>   # CONFIG_VIRTIO_PCI is not set
>   # CONFIG_VIRTIO_SANDBOX is not set
> +CONFIG_TOOLS_MKEFICAPSULE=y
> diff --git a/tools/Kconfig b/tools/Kconfig
> index 91ce8ae3e516..117c921da3fe 100644
> --- a/tools/Kconfig
> +++ b/tools/Kconfig
> @@ -90,4 +90,12 @@ config TOOLS_SHA512
>   	help
>   	  Enable SHA512 support in the tools builds
>
> +config TOOLS_MKEFICAPSULE
> +	bool "Build efimkcapsule command"
> +	default y if EFI_CAPSULE_ON_DISK

We discussed this with Tom before. Building of tools should not depend
on board options. Can we make this 'default y'?

I wonder if a dependency are missing:

With CONFIG_FIT=n './mkeficapsule' shows the usage with:

    -f, --fit <fit image>       new FIT image file

I guess the tool should select:

CONFIG_FIT
CONFIG_FIT_SIGNATURE

And #ifdef CONFIG_FIT_SIGNATURE should be removed in the code.

Best regards

Heinrich

> +	help
> +	  This command allows users to create a UEFI capsule file and,
> +	  optionally sign that file. If you want to enable UEFI capsule
> +	  update feature on your target, you certainly need this.
> +
>   endmenu
> diff --git a/tools/Makefile b/tools/Makefile
> index 1763f44cac43..766c0674f4a0 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -238,8 +238,7 @@ hostprogs-$(CONFIG_MIPS) += mips-relocs
>   hostprogs-$(CONFIG_ASN1_COMPILER)	+= asn1_compiler
>   HOSTCFLAGS_asn1_compiler.o = -idirafter $(srctree)/include
>
> -mkeficapsule-objs	:= mkeficapsule.o $(LIBFDT_OBJS)
> -hostprogs-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += mkeficapsule
> +hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
>
>   # We build some files with extra pedantic flags to try to minimize things
>   # that won't build on some weird host compiler -- though there are lots of
AKASHI Takahiro Jan. 20, 2022, 1:39 a.m. UTC | #2 
Heinrich,

On Wed, Jan 19, 2022 at 05:08:14PM +0100, Heinrich Schuchardt wrote:
> On 1/18/22 05:39, AKASHI Takahiro wrote:
> > Add CONFIG_TOOLS_MKEFICAPSULE. Then we want to always build mkeficapsule
> > if tools-only_defconfig is used.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > ---
> >   configs/tools-only_defconfig | 1 +
> >   tools/Kconfig                | 8 ++++++++
> >   tools/Makefile               | 3 +--
> >   3 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig
> > index f482c9a1c1b0..5427797dd4c3 100644
> > --- a/configs/tools-only_defconfig
> > +++ b/configs/tools-only_defconfig
> > @@ -31,3 +31,4 @@ CONFIG_I2C_EDID=y
> >   # CONFIG_VIRTIO_MMIO is not set
> >   # CONFIG_VIRTIO_PCI is not set
> >   # CONFIG_VIRTIO_SANDBOX is not set
> > +CONFIG_TOOLS_MKEFICAPSULE=y
> > diff --git a/tools/Kconfig b/tools/Kconfig
> > index 91ce8ae3e516..117c921da3fe 100644
> > --- a/tools/Kconfig
> > +++ b/tools/Kconfig
> > @@ -90,4 +90,12 @@ config TOOLS_SHA512
> >   	help
> >   	  Enable SHA512 support in the tools builds
> > 
> > +config TOOLS_MKEFICAPSULE
> > +	bool "Build efimkcapsule command"
> > +	default y if EFI_CAPSULE_ON_DISK
> 
> We discussed this with Tom before. Building of tools should not depend
> on board options. Can we make this 'default y'?

No.
I think we have different opinions here.

I think that any of *board* configs should build only all the binaries
that are need to run U-Boot on that board.
For distros' case that you have mentioned before, we should
encourage them to use tools-only_defconfig for packaging U-Boot
host tools rather than using any particular *board* config.

# In either case, the resulting binary, as far as mkeficapsule is
concerned, is the exact same without any dependency of target configs.

> I wonder if a dependency are missing:
> 
> With CONFIG_FIT=n './mkeficapsule' shows the usage with:
> 
>    -f, --fit <fit image>       new FIT image file
> 
> I guess the tool should select:
> 
> CONFIG_FIT
> CONFIG_FIT_SIGNATURE
> 
> And #ifdef CONFIG_FIT_SIGNATURE should be removed in the code.

I'm not sure what your point is.

I believe that what you and Simon demand in building any host tool
is that the binary be the same whatever target configs are enabled
(or disabled).
So showing "-f, --fit <fit image>       new FIT image file" unconditionally
is a natural consequence.

It doesn't make sense that CONFIG_TOOLS_MKEFICAPSULE selects
any of target configs explicitly.

Furthermore, I don't have "#ifdef CONFIG_FIT_SIGNATURE" in mkeficapsule.c.
# there is some trick around CONFIG_FIT_SIGNATURE in tools/Kconfig, though.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +	help
> > +	  This command allows users to create a UEFI capsule file and,
> > +	  optionally sign that file. If you want to enable UEFI capsule
> > +	  update feature on your target, you certainly need this.
> > +
> >   endmenu
> > diff --git a/tools/Makefile b/tools/Makefile
> > index 1763f44cac43..766c0674f4a0 100644
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -238,8 +238,7 @@ hostprogs-$(CONFIG_MIPS) += mips-relocs
> >   hostprogs-$(CONFIG_ASN1_COMPILER)	+= asn1_compiler
> >   HOSTCFLAGS_asn1_compiler.o = -idirafter $(srctree)/include
> > 
> > -mkeficapsule-objs	:= mkeficapsule.o $(LIBFDT_OBJS)
> > -hostprogs-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += mkeficapsule
> > +hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
> > 
> >   # We build some files with extra pedantic flags to try to minimize things
> >   # that won't build on some weird host compiler -- though there are lots of
>
Tom Rini Jan. 20, 2022, 3:06 p.m. UTC | #3 
On Thu, Jan 20, 2022 at 10:39:03AM +0900, AKASHI Takahiro wrote:
> Heinrich,
> 
> On Wed, Jan 19, 2022 at 05:08:14PM +0100, Heinrich Schuchardt wrote:
> > On 1/18/22 05:39, AKASHI Takahiro wrote:
> > > Add CONFIG_TOOLS_MKEFICAPSULE. Then we want to always build mkeficapsule
> > > if tools-only_defconfig is used.
> > > 
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >   configs/tools-only_defconfig | 1 +
> > >   tools/Kconfig                | 8 ++++++++
> > >   tools/Makefile               | 3 +--
> > >   3 files changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig
> > > index f482c9a1c1b0..5427797dd4c3 100644
> > > --- a/configs/tools-only_defconfig
> > > +++ b/configs/tools-only_defconfig
> > > @@ -31,3 +31,4 @@ CONFIG_I2C_EDID=y
> > >   # CONFIG_VIRTIO_MMIO is not set
> > >   # CONFIG_VIRTIO_PCI is not set
> > >   # CONFIG_VIRTIO_SANDBOX is not set
> > > +CONFIG_TOOLS_MKEFICAPSULE=y
> > > diff --git a/tools/Kconfig b/tools/Kconfig
> > > index 91ce8ae3e516..117c921da3fe 100644
> > > --- a/tools/Kconfig
> > > +++ b/tools/Kconfig
> > > @@ -90,4 +90,12 @@ config TOOLS_SHA512
> > >   	help
> > >   	  Enable SHA512 support in the tools builds
> > > 
> > > +config TOOLS_MKEFICAPSULE
> > > +	bool "Build efimkcapsule command"
> > > +	default y if EFI_CAPSULE_ON_DISK
> > 
> > We discussed this with Tom before. Building of tools should not depend
> > on board options. Can we make this 'default y'?
> 
> No.
> I think we have different opinions here.
> 
> I think that any of *board* configs should build only all the binaries
> that are need to run U-Boot on that board.
> For distros' case that you have mentioned before, we should
> encourage them to use tools-only_defconfig for packaging U-Boot
> host tools rather than using any particular *board* config.
> 
> # In either case, the resulting binary, as far as mkeficapsule is
> concerned, is the exact same without any dependency of target configs.

For "mkimage" we go very very far in the direction of "this tool needs
to be the same for all boards" because so many times not doing so has
come back to be a problem for users and developers and distros.

This is adding a new tool, yes?  We have many examples of only building
a tool given a CONFIG option, and so long as the tool itself doesn't
change based on CONFIG options, that's fine.  tools-only_defconfig needs
to enable this, and is enabling this.
diff mbox series

Patch

diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig
index f482c9a1c1b0..5427797dd4c3 100644
--- a/configs/tools-only_defconfig
+++ b/configs/tools-only_defconfig
@@ -31,3 +31,4 @@  CONFIG_I2C_EDID=y
 # CONFIG_VIRTIO_MMIO is not set
 # CONFIG_VIRTIO_PCI is not set
 # CONFIG_VIRTIO_SANDBOX is not set
+CONFIG_TOOLS_MKEFICAPSULE=y
diff --git a/tools/Kconfig b/tools/Kconfig
index 91ce8ae3e516..117c921da3fe 100644
--- a/tools/Kconfig
+++ b/tools/Kconfig
@@ -90,4 +90,12 @@  config TOOLS_SHA512
 	help
 	  Enable SHA512 support in the tools builds
 
+config TOOLS_MKEFICAPSULE
+	bool "Build efimkcapsule command"
+	default y if EFI_CAPSULE_ON_DISK
+	help
+	  This command allows users to create a UEFI capsule file and,
+	  optionally sign that file. If you want to enable UEFI capsule
+	  update feature on your target, you certainly need this.
+
 endmenu
diff --git a/tools/Makefile b/tools/Makefile
index 1763f44cac43..766c0674f4a0 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -238,8 +238,7 @@  hostprogs-$(CONFIG_MIPS) += mips-relocs
 hostprogs-$(CONFIG_ASN1_COMPILER)	+= asn1_compiler
 HOSTCFLAGS_asn1_compiler.o = -idirafter $(srctree)/include
 
-mkeficapsule-objs	:= mkeficapsule.o $(LIBFDT_OBJS)
-hostprogs-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += mkeficapsule
+hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
 
 # We build some files with extra pedantic flags to try to minimize things
 # that won't build on some weird host compiler -- though there are lots of