Re: [PATCH v2 2/2] spl: Add support for booting from ESP

Heinrich Schuchardt
2024-01-16T13:39:29Z

On 16.01.24 13:36, Mayuresh Chitale wrote:
> Some platforms as described by EBBR specification may store images in
> the FIRMWARE directory of the UEFI system partition(ESP). Add support
> to boot from the EFI system partition if it is enabled for a platform.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>

Thank you for respinning this series.

For future submissions it would be preferable to have the changes
between the versions also in the individual patches.

> ---
> common/spl/Kconfig | 8 +++++++
> common/spl/spl_blk_fs.c | 51 ++++++++++++++++++++++++++++++-----------
> common/spl/spl_fat.c | 34 +++++++++++++++++++++++----
> 3 files changed, 75 insertions(+), 18 deletions(-)
>
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index cf7ffc9b112..48e4e43196a 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -1292,6 +1292,14 @@ config SPL_SATA_RAW_U_BOOT_SECTOR
> Sector on the SATA disk to load U-Boot from, when the SATA disk is being
> used in raw mode. Units: SATA disk sectors (1 sector = 512 bytes).
>
> +config SPL_ESP_BOOT
> + bool "Load next stage boot image from the UEFI system partition"
> + default y if BOOT_DEFAULTS
> + select SPL_PARTITION_TYPE_GUID
> + help
> + When enabled, first try to boot from the UEFI system partition as
> + described in the Ch.4 of the EBBR specification.
> +
> config SPL_NVME
> bool "NVM Express device support"
> depends on BLK
> diff --git a/common/spl/spl_blk_fs.c b/common/spl/spl_blk_fs.c
> index 04eac6f306b..a2e8c2ce910 100644
> --- a/common/spl/spl_blk_fs.c
> +++ b/common/spl/spl_blk_fs.c
> @@ -10,12 +10,15 @@
> #include <spl_load.h>
> #include <image.h>
> #include <fs.h>
> +#include <part.h>
> #include <asm/cache.h>
> #include <asm/io.h>
>
> struct blk_dev {
> const char *ifname;
> const char *filename;
> + int devnum;
> + int partnum;
> char dev_part_str[8];
> };
>
> @@ -44,6 +47,29 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
> return actlen;
> }
>
> +static int spl_blk_file_size(struct blk_dev *dev, const char *filename,
> + loff_t *filesize)
> +{
> + int ret;
> +
> + snprintf(dev->dev_part_str, sizeof(dev->dev_part_str) - 1, "%x:%x",
> + dev->devnum, dev->partnum);

Using log functions allows to provide more information if
CONFIG_SPL_LOG=y otherwise they fall back to printf() and debug().

> + debug("Loading file %s from %s %s\n", filename, dev->ifname,
> + dev->dev_part_str);

log_debug(

> + ret = fs_set_blk_dev(dev->ifname, dev->dev_part_str, FS_TYPE_ANY);
> + if (ret) {
> + printf("spl: unable to set blk_dev %s %s. Err - %d\n",

SPL binary size is limited on many systems. There is already a message
telling that we are in SPL. I would suggest to abbreviate the message.

log_err("Can't access %s %s\n", dev->ifname, dev->dev_part_str);

> + dev->ifname, dev->dev_part_str, ret);
> + return ret;
> + }
> +
> + ret = fs_size(filename, filesize);
> + if (ret)
> + printf("spl: unable to get size, file: %s. Err - %d\n",
> + filename, ret);

log_err("File not found %s\n", filename);

Moving to log functions and adjusting the messages can be done in a
follow up patch. I don't want to stop this series.

Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

> + return ret;
> +}
> +
> int spl_blk_load_image(struct spl_image_info *spl_image,
> struct spl_boot_device *bootdev,
> enum uclass_id uclass_id, int devnum, int partnum)
> @@ -53,7 +79,7 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
> loff_t filesize;
> struct blk_dev dev;
> struct spl_load_info load;
> - int ret;
> + int ret, part;
>
> blk_desc = blk_get_devnum_by_uclass_id(uclass_id, devnum);
> if (!blk_desc) {
> @@ -65,21 +91,18 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
>
> dev.filename = filename;
> dev.ifname = blk_get_uclass_name(uclass_id);
> - snprintf(dev.dev_part_str, sizeof(dev.dev_part_str) - 1, "%x:%x",
> - devnum, partnum);
> - ret = fs_set_blk_dev(dev.ifname, dev.dev_part_str, FS_TYPE_ANY);
> - if (ret) {
> - printf("spl: unable to set blk_dev %s %s. Err - %d\n",
> - dev.ifname, dev.dev_part_str, ret);
> - return ret;
> + dev.devnum = devnum;
> + dev.partnum = partnum;
> + if (IS_ENABLED(CONFIG_SPL_ESP_BOOT)) {
> + part = part_get_esp(blk_desc);
> + if (part)
> + dev.partnum = part;
> + else
> + return -ENODEV;
> }
> -
> - ret = fs_size(filename, &filesize);
> - if (ret) {
> - printf("spl: unable to get file size: %s. Err - %d\n",
> - filename, ret);
> + ret = spl_blk_file_size(&dev, filename, &filesize);
> + if (ret)
> return ret;
> - }
>
> load.read = spl_fit_read;
> if (IS_ENABLED(CONFIG_SPL_FS_FAT_DMA_ALIGN))
> diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
> index a52f9e178e6..8c426a3f3e7 100644
> --- a/common/spl/spl_fat.c
> +++ b/common/spl/spl_fat.c
> @@ -60,10 +60,10 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
> return actread;
> }
>
> -int spl_load_image_fat(struct spl_image_info *spl_image,
> - struct spl_boot_device *bootdev,
> - struct blk_desc *block_dev, int partition,
> - const char *filename)
> +int spl_load_image_fat_one(struct spl_image_info *spl_image,
> + struct spl_boot_device *bootdev,
> + struct blk_desc *block_dev, int partition,
> + const char *filename)
> {
> int err;
> loff_t size;
> @@ -103,6 +103,32 @@ end:
> return err;
> }
>
> +int spl_load_image_fat(struct spl_image_info *spl_image,
> + struct spl_boot_device *bootdev,
> + struct blk_desc *block_dev, int partition,
> + const char *filename)
> +{
> + int err, part;
> +
> + /*
> + * First try to boot from EFI System partition. In case of failure,
> + * fall back to the configured partition.
> + */
> + if (IS_ENABLED(CONFIG_SPL_ESP_BOOT)) {
> + part = part_get_esp(block_dev);
> + if (part) {
> + err = spl_load_image_fat_one(spl_image, bootdev,
> + block_dev, part,
> + filename);
> + if (!err)
> + return err;
> + }
> + }
> +
> + return spl_load_image_fat_one(spl_image, bootdev, block_dev,
> + partition, filename);
> +}
> +
> #if CONFIG_IS_ENABLED(OS_BOOT)
> int spl_load_image_fat_os(struct spl_image_info *spl_image,
> struct spl_boot_device *bootdev,