Re: [Buildroot] [PATCH 1/1] package/rpi-eeprom: New package for RPi bootloader tools

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Tim,

Thanks a lot for your submission, and sorry for the delay in getting
back to you! See below some feedback.

On Mon, 16 Dec 2024 12:58:00 +0000
Tim Gover via buildroot <buildroot@buildroot.org> wrote:

> This package adds the host and OS tools for configuring
> the bootloader and building/signing tools for signed boot.
> 
> Normally, the host tools would be invoked from a post-build
> script to sign the boot.img file. The device tools would be
> invoked from a custom on-device software update process e.g.
> using rpi-eeprom-digest to verify the signature of a boot.img
> file against a public key retrieved from the EEPROM.

Thanks for the good explanation of why both a target and host package
are needed.

>  configs/raspberrypi5_defconfig     |  1 +
>  package/Config.in                  |  1 +
>  package/Config.in.host             |  1 +
>  package/rpi-eeprom/Config.in       |  9 +++++++++
>  package/rpi-eeprom/Config.in.host  |  9 +++++++++
>  package/rpi-eeprom/rpi-eeprom.hash |  1 +
>  package/rpi-eeprom/rpi-eeprom.mk   | 27 +++++++++++++++++++++++++++
>  7 files changed, 49 insertions(+)
>  create mode 100644 package/rpi-eeprom/Config.in
>  create mode 100644 package/rpi-eeprom/Config.in.host
>  create mode 100644 package/rpi-eeprom/rpi-eeprom.hash
>  create mode 100644 package/rpi-eeprom/rpi-eeprom.mk

Could you please add an entry in the DEVELOPERS file for this new
package?

> diff --git a/configs/raspberrypi5_defconfig b/configs/raspberrypi5_defconfig
> index 8cbd533eee..6d364ad32f 100644
> --- a/configs/raspberrypi5_defconfig
> +++ b/configs/raspberrypi5_defconfig
> @@ -31,3 +31,4 @@ BR2_PACKAGE_HOST_DOSFSTOOLS=y
>  BR2_PACKAGE_HOST_GENIMAGE=y
>  BR2_PACKAGE_HOST_KMOD_XZ=y
>  BR2_PACKAGE_HOST_MTOOLS=y
> +BR2_PACKAGE_HOST_RPI_EEPROM=y

I'd prefer to see this done as a separate commit (possibly in the same
patch series). However, the bigger question is why is this only done
for RPi5, and not the other RaspberryPi? Is there anything that makes
this tool RPi5-specific?

> diff --git a/package/rpi-eeprom/Config.in b/package/rpi-eeprom/Config.in
> new file mode 100644
> index 0000000000..02e1109264
> --- /dev/null
> +++ b/package/rpi-eeprom/Config.in
> @@ -0,0 +1,9 @@
> +config BR2_PACKAGE_RPI_EEPROM
> +	bool "rpi-eeprom"
> +	select BR2_PACKAGE_LIBOPENSSL

Why is openssl needed? You don't even depend on it in the .mk file.

Also, you should directly select BR2_PACKAGE_LIBOPENSSL. But you should
select BR2_PACKAGE_OPENSSL, and then if only openssl is supported (and
not libressl), you also need to select
BR2_PACKAGE_OPENSSL_FORCE_LIBOPENSSL. openssl is kind of a special
beast, as it's a virtual package with two implementations: libopenssl
and libressl.

> +	select BR2_PACKAGE_PYTHON3
> +	select BR2_PACKAGE_PYTHON_PYCRYPTODOMEX

Are these runtime dependencies? If so:

	select BR2_PACKAGE_PYTHON3 # runtime
	select BR2_PACKAGE_PYTHON_PYCRYPTODOMEX # runtime

> +	help
> +	  Raspberry Pi bootloader tools.

A slightly longer help text with more details would make sense, perhaps
borrowing some details from your commit log on the use case (here for
the target package).

> +	  https://github.com/raspberrypi/rpi-eeprom
> diff --git a/package/rpi-eeprom/Config.in.host b/package/rpi-eeprom/Config.in.host
> new file mode 100644
> index 0000000000..3d36d7bec8
> --- /dev/null
> +++ b/package/rpi-eeprom/Config.in.host
> @@ -0,0 +1,9 @@
> +config BR2_PACKAGE_HOST_RPI_EEPROM
> +	bool "host rpi-eeprom"
> +	select BR2_PACKAGE_LIBOPENSSL
> +	select BR2_PACKAGE_PYTHON3
> +	select BR2_PACKAGE_PYTHON_PYCRYPTODOMEX

These selects don't make sense, as you're selecting target packages...
while we're here in the option enabling the host variant of rpi-eeprom.
Most likely you don't need any "select" here.

> +	help
> +	  Raspberry Pi bootloader tools.
> +
> +	  https://github.com/raspberrypi/rpi-eeprom
> diff --git a/package/rpi-eeprom/rpi-eeprom.hash b/package/rpi-eeprom/rpi-eeprom.hash
> new file mode 100644
> index 0000000000..27cd0bfdf3
> --- /dev/null
> +++ b/package/rpi-eeprom/rpi-eeprom.hash
> @@ -0,0 +1 @@
> +sha256  90970fd9a72c29449a8f4f27577395a1fb418f87b979adeb81d51300f959dab9  rpi-eeprom-fe7bfc720165464d9dfe2f85fe090ca22a625bd7.tar.gz

Please add a comment on where this hash comes from. Most likely:

# Locally calculated

Also, please add a hash for the LICENSE file (see other packages).

> diff --git a/package/rpi-eeprom/rpi-eeprom.mk b/package/rpi-eeprom/rpi-eeprom.mk
> new file mode 100644
> index 0000000000..db1634315f
> --- /dev/null
> +++ b/package/rpi-eeprom/rpi-eeprom.mk
> @@ -0,0 +1,27 @@
> +################################################################################
> +#
> +# rpi-eeprom
> +#
> +################################################################################
> +
> +RPI_EEPROM_VERSION = fe7bfc720165464d9dfe2f85fe090ca22a625bd7
> +RPI_EEPROM_SITE = $(call github,raspberrypi,rpi-eeprom,$(RPI_EEPROM_VERSION))
> +RPI_EEPROM_LICENSE = BSD-3-Clause
> +RPI_EEPROM_LICENSE_FILES = LICENSE
> +
> +HOST_RPI_EEPROM_INSTALL = YES
> +RPI_EEPROM_INSTALL = YES

Neither of these are needed/have an effect.

You will most likely need:

HOST_RPI_EEPROM_DEPENDENCIES = host-python3 host-python-pycryptodomex

indeed, for the host package, we need the package to be fully
functional right after its installation, as other packages depending on
it might use it right after.

However, for the target package, we only care that everything is on the
target once it runs, hence select BR2_PACKAGE_PYTHON3 + select
BR2_PACKAGE_PYTHON_PYCRYPTODOMEX in Config.in is sufficient.

Could you have a look at adjusting those details and submitting a
second iteration of this patch?

Once again, thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot