[PATCH v2 0/2] SPL EBBR - EFI System Partition support

[PATCH v2 0/2] SPL EBBR - EFI System Partition support

[Mayuresh Chitale]

This series adds support to locate an EFI System Partition on a disk and
boot the next stage from such a parition if found. The next stage image
is expected to be under the FIRMWARE directory as described in the EBBR
specification [1]. Also update the spl_blk_fs and spl_fat drivers to
prefer booting from ESP and fall back to the configured parition in
case of a failure.

Changes in v2:
- Rebase on latest next branch
- Enable SPL_ESP_BOOT by default
- For NVMe boot, remove fallback in case of ESP boot failure

Mayuresh Chitale (2):
  part: Add a function to find ESP partition
  spl: Add support for booting from ESP

 common/spl/Kconfig      |  8 +++++++
 common/spl/spl_blk_fs.c | 51 ++++++++++++++++++++++++++++++-----------
 common/spl/spl_fat.c    | 34 +++++++++++++++++++++++----
 disk/part.c             | 16 +++++++++++++
 include/part.h          | 11 +++++++++
 5 files changed, 102 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] part: Add a function to find ESP partition

[Mayuresh Chitale]

If a disk has an EFI system partition (ESP) then it can be used to
locate the boot files. Add a function to find the ESP.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 disk/part.c    | 16 ++++++++++++++++
 include/part.h | 11 +++++++++++
 2 files changed, 27 insertions(+)

diff --git a/disk/part.c b/disk/part.c
index 36b88205eca..6b1fbc18637 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -848,3 +848,19 @@ int part_get_bootable(struct blk_desc *desc)
 
 	return 0;
 }
+
+int part_get_esp(struct blk_desc *desc)
+{
+	struct disk_partition info;
+	int p;
+
+	for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
+		int ret;
+
+		ret = part_get_info(desc, p, &info);
+		if (!ret && (info.bootable & PART_EFI_SYSTEM_PARTITION))
+			return p;
+	}
+
+	return 0;
+}
diff --git a/include/part.h b/include/part.h
index db34bc6bb7d..30e049c8f19 100644
--- a/include/part.h
+++ b/include/part.h
@@ -690,6 +690,14 @@ int part_get_type_by_name(const char *name);
  */
 int part_get_bootable(struct blk_desc *desc);
 
+/**
+ * part_get_esp() - Find the EFI system partition
+ *
+ * @desc: Block-device descriptor
+ * @Return the EFI system partition, or 0 if there is none
+ */
+int part_get_esp(struct blk_desc *desc);
+
 #else
 static inline int part_driver_get_count(void)
 { return 0; }
@@ -700,6 +708,9 @@ static inline struct part_driver *part_driver_get_first(void)
 static inline bool part_get_bootable(struct blk_desc *desc)
 { return false; }
 
+static inline bool part_get_esp(struct blk_desc *desc)
+{ return false; }
+
 #endif /* CONFIG_PARTITIONS */
 
 #endif /* _PART_H */
-- 
2.34.1

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

[Mayuresh Chitale]

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>
---
 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);
+	debug("Loading file %s from %s %s\n", filename, dev->ifname,
+	      dev->dev_part_str);
+	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;
+	}
+
+	ret = fs_size(filename, filesize);
+	if (ret)
+		printf("spl: unable to get size, file: %s. Err - %d\n",
+		       filename, ret);
+	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,
-- 
2.34.1

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

[Heinrich Schuchardt]

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,

Re: [PATCH v2 1/2] part: Add a function to find ESP partition

[Heinrich Schuchardt]

On 16.01.24 13:36, Mayuresh Chitale wrote:
> If a disk has an EFI system partition (ESP) then it can be used to
> locate the boot files. Add a function to find the ESP.
> 
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>   disk/part.c    | 16 ++++++++++++++++
>   include/part.h | 11 +++++++++++
>   2 files changed, 27 insertions(+)
> 
> diff --git a/disk/part.c b/disk/part.c
> index 36b88205eca..6b1fbc18637 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -848,3 +848,19 @@ int part_get_bootable(struct blk_desc *desc)
>   
>   	return 0;
>   }
> +
> +int part_get_esp(struct blk_desc *desc)
> +{
> +	struct disk_partition info;
> +	int p;
> +
> +	for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> +		int ret;
> +
> +		ret = part_get_info(desc, p, &info);
> +		if (!ret && (info.bootable & PART_EFI_SYSTEM_PARTITION))
> +			return p;
> +	}
> +
> +	return 0;
> +}
> diff --git a/include/part.h b/include/part.h
> index db34bc6bb7d..30e049c8f19 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -690,6 +690,14 @@ int part_get_type_by_name(const char *name);
>    */
>   int part_get_bootable(struct blk_desc *desc);
>   
> +/**
> + * part_get_esp() - Find the EFI system partition
> + *
> + * @desc: Block-device descriptor
> + * @Return the EFI system partition, or 0 if there is none

We want to be able to add the include to our API documentation. This 
requires adhering to the Sphinx documentation style.

%s/@Return the/Return:/

Cf. 
https://www.kernel.org/doc/html/v6.7/doc-guide/kernel-doc.html#function-documentation

Best regards

Heinrich

> + */
> +int part_get_esp(struct blk_desc *desc);
> +
>   #else
>   static inline int part_driver_get_count(void)
>   { return 0; }
> @@ -700,6 +708,9 @@ static inline struct part_driver *part_driver_get_first(void)
>   static inline bool part_get_bootable(struct blk_desc *desc)
>   { return false; }
>   
> +static inline bool part_get_esp(struct blk_desc *desc)
> +{ return false; }
> +
>   #endif /* CONFIG_PARTITIONS */
>   
>   #endif /* _PART_H */

Re: [PATCH v2 1/2] part: Add a function to find ESP partition

[Heinrich Schuchardt]

On 16.01.24 14:45, Heinrich Schuchardt wrote:
> On 16.01.24 13:36, Mayuresh Chitale wrote:
>> If a disk has an EFI system partition (ESP) then it can be used to
>> locate the boot files. Add a function to find the ESP.
>>
>> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
>> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

I ran your patches through Gitlab CI and some issues came up:

https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/771497
https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/771498

Could you, please, have a look at it.

Best regards

Heinrich

>> ---
>>   disk/part.c    | 16 ++++++++++++++++
>>   include/part.h | 11 +++++++++++
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/disk/part.c b/disk/part.c
>> index 36b88205eca..6b1fbc18637 100644
>> --- a/disk/part.c
>> +++ b/disk/part.c
>> @@ -848,3 +848,19 @@ int part_get_bootable(struct blk_desc *desc)
>>       return 0;
>>   }
>> +
>> +int part_get_esp(struct blk_desc *desc)
>> +{
>> +    struct disk_partition info;
>> +    int p;
>> +
>> +    for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
>> +        int ret;
>> +
>> +        ret = part_get_info(desc, p, &info);
>> +        if (!ret && (info.bootable & PART_EFI_SYSTEM_PARTITION))
>> +            return p;
>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/include/part.h b/include/part.h
>> index db34bc6bb7d..30e049c8f19 100644
>> --- a/include/part.h
>> +++ b/include/part.h
>> @@ -690,6 +690,14 @@ int part_get_type_by_name(const char *name);
>>    */
>>   int part_get_bootable(struct blk_desc *desc);
>> +/**
>> + * part_get_esp() - Find the EFI system partition
>> + *
>> + * @desc: Block-device descriptor
>> + * @Return the EFI system partition, or 0 if there is none
> 
> We want to be able to add the include to our API documentation. This 
> requires adhering to the Sphinx documentation style.
> 
> %s/@Return the/Return:/
> 
> Cf. 
> https://www.kernel.org/doc/html/v6.7/doc-guide/kernel-doc.html#function-documentation
> 
> Best regards
> 
> Heinrich
> 
>> + */
>> +int part_get_esp(struct blk_desc *desc);
>> +
>>   #else
>>   static inline int part_driver_get_count(void)
>>   { return 0; }
>> @@ -700,6 +708,9 @@ static inline struct part_driver 
>> *part_driver_get_first(void)
>>   static inline bool part_get_bootable(struct blk_desc *desc)
>>   { return false; }
>> +static inline bool part_get_esp(struct blk_desc *desc)
>> +{ return false; }
>> +
>>   #endif /* CONFIG_PARTITIONS */
>>   #endif /* _PART_H */
> 

Re: [PATCH v2 1/2] part: Add a function to find ESP partition

[Sean Anderson]

On 1/16/24 07:36, Mayuresh Chitale wrote:
> If a disk has an EFI system partition (ESP) then it can be used to
> locate the boot files. Add a function to find the ESP.
> 
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>   disk/part.c    | 16 ++++++++++++++++
>   include/part.h | 11 +++++++++++
>   2 files changed, 27 insertions(+)
> 
> diff --git a/disk/part.c b/disk/part.c
> index 36b88205eca..6b1fbc18637 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -848,3 +848,19 @@ int part_get_bootable(struct blk_desc *desc)
>   
>   	return 0;
>   }
> +
> +int part_get_esp(struct blk_desc *desc)
> +{
> +	struct disk_partition info;
> +	int p;
> +
> +	for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> +		int ret;
> +
> +		ret = part_get_info(desc, p, &info);
> +		if (!ret && (info.bootable & PART_EFI_SYSTEM_PARTITION))
> +			return p;
> +	}
> +
> +	return 0;
> +}
> diff --git a/include/part.h b/include/part.h
> index db34bc6bb7d..30e049c8f19 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -690,6 +690,14 @@ int part_get_type_by_name(const char *name);
>    */
>   int part_get_bootable(struct blk_desc *desc);
>   
> +/**
> + * part_get_esp() - Find the EFI system partition
> + *
> + * @desc: Block-device descriptor
> + * @Return the EFI system partition, or 0 if there is none
> + */
> +int part_get_esp(struct blk_desc *desc);
> +
>   #else
>   static inline int part_driver_get_count(void)
>   { return 0; }
> @@ -700,6 +708,9 @@ static inline struct part_driver *part_driver_get_first(void)
>   static inline bool part_get_bootable(struct blk_desc *desc)
>   { return false; }
>   
> +static inline bool part_get_esp(struct blk_desc *desc)

Please use the same signature as above.

> +{ return false; }

The statement here should be on its own line.

--Sean

> +
>   #endif /* CONFIG_PARTITIONS */
>   
>   #endif /* _PART_H */

Re: [PATCH v2 1/2] part: Add a function to find ESP partition

[mchitale]

Hi Heinrich,

On Tue, 2024-01-16 at 17:14 +0100, Heinrich Schuchardt wrote:
> On 16.01.24 14:45, Heinrich Schuchardt wrote:
> > On 16.01.24 13:36, Mayuresh Chitale wrote:
> > > If a disk has an EFI system partition (ESP) then it can be used
> > > to
> > > locate the boot files. Add a function to find the ESP.
> > > 
> > > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > > Reviewed-by: Heinrich Schuchardt <
> > > heinrich.schuchardt@canonical.com>
> 
> I ran your patches through Gitlab CI and some issues came up:
> 
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/771497
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/771498
> 
> Could you, please, have a look at it.

I looked into the log files below:
https://u-boot.source-pages.denx.de/-/custodians/u-boot-efi/-/jobs/771497/artifacts/test-log.html
https://u-boot.source-pages.denx.de/-/custodians/u-boot-efi/-/jobs/771498/artifacts/test-log.html

If those logs are correct, I think the failure is probably because I
removed fallback to the user provided partition in case the ESP probe
failed for some reason. I think that fallback is required here. 
> 
> Best regards
> 
> Heinrich
> 
> > > ---
> > >   disk/part.c    | 16 ++++++++++++++++
> > >   include/part.h | 11 +++++++++++
> > >   2 files changed, 27 insertions(+)
> > > 
> > > diff --git a/disk/part.c b/disk/part.c
> > > index 36b88205eca..6b1fbc18637 100644
> > > --- a/disk/part.c
> > > +++ b/disk/part.c
> > > @@ -848,3 +848,19 @@ int part_get_bootable(struct blk_desc *desc)
> > >       return 0;
> > >   }
> > > +
> > > +int part_get_esp(struct blk_desc *desc)
> > > +{
> > > +    struct disk_partition info;
> > > +    int p;
> > > +
> > > +    for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> > > +        int ret;
> > > +
> > > +        ret = part_get_info(desc, p, &info);
> > > +        if (!ret && (info.bootable & PART_EFI_SYSTEM_PARTITION))
> > > +            return p;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > diff --git a/include/part.h b/include/part.h
> > > index db34bc6bb7d..30e049c8f19 100644
> > > --- a/include/part.h
> > > +++ b/include/part.h
> > > @@ -690,6 +690,14 @@ int part_get_type_by_name(const char *name);
> > >    */
> > >   int part_get_bootable(struct blk_desc *desc);
> > > +/**
> > > + * part_get_esp() - Find the EFI system partition
> > > + *
> > > + * @desc: Block-device descriptor
> > > + * @Return the EFI system partition, or 0 if there is none
> > 
> > We want to be able to add the include to our API documentation.
> > This 
> > requires adhering to the Sphinx documentation style.
> > 
> > %s/@Return the/Return:/
> > 
> > Cf. 
> > https://www.kernel.org/doc/html/v6.7/doc-guide/kernel-doc.html#function-documentation
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > > + */
> > > +int part_get_esp(struct blk_desc *desc);
> > > +
> > >   #else
> > >   static inline int part_driver_get_count(void)
> > >   { return 0; }
> > > @@ -700,6 +708,9 @@ static inline struct part_driver 
> > > *part_driver_get_first(void)
> > >   static inline bool part_get_bootable(struct blk_desc *desc)
> > >   { return false; }
> > > +static inline bool part_get_esp(struct blk_desc *desc)
> > > +{ return false; }
> > > +
> > >   #endif /* CONFIG_PARTITIONS */
> > >   #endif /* _PART_H */

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

[Mayuresh Chitale]

On Tue, Jan 16, 2024 at 7:09 PM Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> 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.
>
Ok.

>
> > ---
> >   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.
>
> Ok.

> 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,
>
>

Re: [PATCH v2 1/2] part: Add a function to find ESP partition

[Mayuresh Chitale]

On Tue, Jan 16, 2024 at 7:15 PM Heinrich Schuchardt <
heinrich.schuchardt@canonical.com> wrote:

> On 16.01.24 13:36, Mayuresh Chitale wrote:
> > If a disk has an EFI system partition (ESP) then it can be used to
> > locate the boot files. Add a function to find the ESP.
> >
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > ---
> >   disk/part.c    | 16 ++++++++++++++++
> >   include/part.h | 11 +++++++++++
> >   2 files changed, 27 insertions(+)
> >
> > diff --git a/disk/part.c b/disk/part.c
> > index 36b88205eca..6b1fbc18637 100644
> > --- a/disk/part.c
> > +++ b/disk/part.c
> > @@ -848,3 +848,19 @@ int part_get_bootable(struct blk_desc *desc)
> >
> >       return 0;
> >   }
> > +
> > +int part_get_esp(struct blk_desc *desc)
> > +{
> > +     struct disk_partition info;
> > +     int p;
> > +
> > +     for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> > +             int ret;
> > +
> > +             ret = part_get_info(desc, p, &info);
> > +             if (!ret && (info.bootable & PART_EFI_SYSTEM_PARTITION))
> > +                     return p;
> > +     }
> > +
> > +     return 0;
> > +}
> > diff --git a/include/part.h b/include/part.h
> > index db34bc6bb7d..30e049c8f19 100644
> > --- a/include/part.h
> > +++ b/include/part.h
> > @@ -690,6 +690,14 @@ int part_get_type_by_name(const char *name);
> >    */
> >   int part_get_bootable(struct blk_desc *desc);
> >
> > +/**
> > + * part_get_esp() - Find the EFI system partition
> > + *
> > + * @desc: Block-device descriptor
> > + * @Return the EFI system partition, or 0 if there is none
>
> We want to be able to add the include to our API documentation. This
> requires adhering to the Sphinx documentation style.
>
> %s/@Return the/Return:/
>
> Ok.

> Cf.
>
> https://www.kernel.org/doc/html/v6.7/doc-guide/kernel-doc.html#function-documentation
>
> Best regards
>
> Heinrich
>
> > + */
> > +int part_get_esp(struct blk_desc *desc);
> > +
> >   #else
> >   static inline int part_driver_get_count(void)
> >   { return 0; }
> > @@ -700,6 +708,9 @@ static inline struct part_driver
> *part_driver_get_first(void)
> >   static inline bool part_get_bootable(struct blk_desc *desc)
> >   { return false; }
> >
> > +static inline bool part_get_esp(struct blk_desc *desc)
> > +{ return false; }
> > +
> >   #endif /* CONFIG_PARTITIONS */
> >
> >   #endif /* _PART_H */
>
>

Re: [PATCH v2 1/2] part: Add a function to find ESP partition

[Mayuresh Chitale]

On Wed, Jan 17, 2024 at 6:43 AM Sean Anderson <seanga2@gmail.com> wrote:

> On 1/16/24 07:36, Mayuresh Chitale wrote:
> > If a disk has an EFI system partition (ESP) then it can be used to
> > locate the boot files. Add a function to find the ESP.
> >
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > ---
> >   disk/part.c    | 16 ++++++++++++++++
> >   include/part.h | 11 +++++++++++
> >   2 files changed, 27 insertions(+)
> >
> > diff --git a/disk/part.c b/disk/part.c
> > index 36b88205eca..6b1fbc18637 100644
> > --- a/disk/part.c
> > +++ b/disk/part.c
> > @@ -848,3 +848,19 @@ int part_get_bootable(struct blk_desc *desc)
> >
> >       return 0;
> >   }
> > +
> > +int part_get_esp(struct blk_desc *desc)
> > +{
> > +     struct disk_partition info;
> > +     int p;
> > +
> > +     for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> > +             int ret;
> > +
> > +             ret = part_get_info(desc, p, &info);
> > +             if (!ret && (info.bootable & PART_EFI_SYSTEM_PARTITION))
> > +                     return p;
> > +     }
> > +
> > +     return 0;
> > +}
> > diff --git a/include/part.h b/include/part.h
> > index db34bc6bb7d..30e049c8f19 100644
> > --- a/include/part.h
> > +++ b/include/part.h
> > @@ -690,6 +690,14 @@ int part_get_type_by_name(const char *name);
> >    */
> >   int part_get_bootable(struct blk_desc *desc);
> >
> > +/**
> > + * part_get_esp() - Find the EFI system partition
> > + *
> > + * @desc: Block-device descriptor
> > + * @Return the EFI system partition, or 0 if there is none
> > + */
> > +int part_get_esp(struct blk_desc *desc);
> > +
> >   #else
> >   static inline int part_driver_get_count(void)
> >   { return 0; }
> > @@ -700,6 +708,9 @@ static inline struct part_driver
> *part_driver_get_first(void)
> >   static inline bool part_get_bootable(struct blk_desc *desc)
> >   { return false; }
> >
> > +static inline bool part_get_esp(struct blk_desc *desc)
>
> Please use the same signature as above.
>
Yes.

> > +{ return false; }
>
> The statement here should be on its own line.
>
> Ok.

> --Sean
>
> > +
> >   #endif /* CONFIG_PARTITIONS */
> >
> >   #endif /* _PART_H */
>
>