[PATCH v2 0/2] efi_loader: identify EFI system partition

[PATCH v2 0/2] efi_loader: identify EFI system partition

[Heinrich Schuchardt]

For the implementation of capsule updates we need to know where the EFI
system partition is located.

With the patches the first available EFI system partition is determined
both for MBR and GPT partition tables.

v2:
	used BIT() macro to define bit mask

Heinrich Schuchardt (2):
  part: detect EFI system partition
  efi_loader: identify EFI system partition

 disk/part_dos.c           | 10 ++++++++--
 disk/part_efi.c           | 12 ++++++++----
 include/efi_loader.h      |  7 +++++++
 include/part.h            | 11 ++++++++++-
 lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
 5 files changed, 53 insertions(+), 7 deletions(-)

--
2.25.1

[PATCH v2 1/2] part: detect EFI system partition

[Heinrich Schuchardt]

Up to now for MBR and GPT partitions the info field 'bootable' was set to 1
if either the partition was an EFI system partition or the bootable flag
was set.

Turn info field 'bootable' into a bit mask with separate bits for bootable
and EFI system partition.

This will allow us to identify the EFI system partition in the UEFI
sub-system.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2:
	used BIT() macro to define bit mask
---
 disk/part_dos.c | 10 ++++++++--
 disk/part_efi.c | 12 ++++++++----
 include/part.h  | 11 ++++++++++-
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/disk/part_dos.c b/disk/part_dos.c
index 83ff40d310..0ec7f1628e 100644
--- a/disk/part_dos.c
+++ b/disk/part_dos.c
@@ -45,9 +45,15 @@ static inline int is_extended(int part_type)
 	    part_type == 0x85);
 }

-static inline int is_bootable(dos_partition_t *p)
+static int is_bootable(dos_partition_t *p)
 {
-	return (p->sys_ind == 0xef) || (p->boot_ind == 0x80);
+	int ret = 0;
+
+	if (p->sys_ind == 0xef)
+		ret |= PART_EFI_SYSTEM_PARTITION;
+	if (p->boot_ind == 0x80)
+		ret |= PART_BOOTABLE;
+	return ret;
 }

 static void print_one_part(dos_partition_t *p, lbaint_t ext_part_sector,
diff --git a/disk/part_efi.c b/disk/part_efi.c
index b2e157d9c1..19f1f43f4e 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -71,11 +71,15 @@ static char *print_efiname(gpt_entry *pte)

 static const efi_guid_t system_guid = PARTITION_SYSTEM_GUID;

-static inline int is_bootable(gpt_entry *p)
+static int is_bootable(gpt_entry *p)
 {
-	return p->attributes.fields.legacy_bios_bootable ||
-		!memcmp(&(p->partition_type_guid), &system_guid,
-			sizeof(efi_guid_t));
+	int ret = 0;
+
+	if (!memcmp(&p->partition_type_guid, &system_guid, sizeof(efi_guid_t)))
+		ret |=  PART_EFI_SYSTEM_PARTITION;
+	if (p->attributes.fields.legacy_bios_bootable)
+		ret |=  PART_BOOTABLE;
+	return ret;
 }

 static int validate_gpt_header(gpt_header *gpt_h, lbaint_t lba,
diff --git a/include/part.h b/include/part.h
index 0b5cf3d5e8..f3442ef85d 100644
--- a/include/part.h
+++ b/include/part.h
@@ -51,13 +51,22 @@ struct block_drvr {
 #define PART_TYPE_LEN 32
 #define MAX_SEARCH_PARTITIONS 64

+#define PART_BOOTABLE			BIT(0)
+#define PART_EFI_SYSTEM_PARTITION	BIT(1)
+
 typedef struct disk_partition {
 	lbaint_t	start;	/* # of first block in partition	*/
 	lbaint_t	size;	/* number of blocks in partition	*/
 	ulong	blksz;		/* block size in bytes			*/
 	uchar	name[PART_NAME_LEN];	/* partition name			*/
 	uchar	type[PART_TYPE_LEN];	/* string type description		*/
-	int	bootable;	/* Active/Bootable flag is set		*/
+	/*
+	 * The bootable is a bitmask with the following fields:
+	 *
+	 * PART_BOOTABLE		the MBR bootable flag is set
+	 * PART_EFI_SYSTEM_PARTITION	the partition is an EFI system partition
+	 */
+	int	bootable;
 #if CONFIG_IS_ENABLED(PARTITION_UUIDS)
 	char	uuid[UUID_STR_LEN + 1];	/* filesystem UUID as string, if exists	*/
 #endif
--
2.25.1

[PATCH v2 2/2] efi_loader: identify EFI system partition

[Heinrich Schuchardt]

For capsule updates we need to identify the EFI system partition.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2:
	no change
---
 include/efi_loader.h      |  7 +++++++
 lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 3f2792892f..4a45033476 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -45,6 +45,13 @@ static inline void *guidcpy(void *dst, const void *src)
 /* Root node */
 extern efi_handle_t efi_root;

+/* EFI system partition */
+extern struct efi_system_partition {
+	enum if_type if_type;
+	int devnum;
+	u8 part;
+} efi_system_partition;
+
 int __efi_entry_check(void);
 int __efi_exit_check(void);
 const char *__efi_nesting(void);
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index fc0682bc48..2f752a5e99 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -13,6 +13,8 @@
 #include <part.h>
 #include <malloc.h>

+struct efi_system_partition efi_system_partition;
+
 const efi_guid_t efi_block_io_guid = EFI_BLOCK_IO_PROTOCOL_GUID;

 /**
@@ -372,6 +374,24 @@ static efi_status_t efi_disk_add_dev(
 	diskobj->ops.media = &diskobj->media;
 	if (disk)
 		*disk = diskobj;
+
+	/* Store first EFI system partition */
+	if (part && !efi_system_partition.if_type) {
+		int r;
+		disk_partition_t info;
+
+		r = part_get_info(desc, part, &info);
+		if (r)
+			return EFI_DEVICE_ERROR;
+		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
+			efi_system_partition.if_type = desc->if_type;
+			efi_system_partition.devnum = desc->devnum;
+			efi_system_partition.part = part;
+			EFI_PRINT("EFI system partition: %s %d:%d\n",
+				  blk_get_if_type_name(desc->if_type),
+				  desc->devnum, part);
+		}
+	}
 	return EFI_SUCCESS;
 }

--
2.25.1

[PATCH v2 2/2] efi_loader: identify EFI system partition

[AKASHI Takahiro]

Heinrich,

On Sun, Apr 05, 2020 at 11:28:18AM +0200, Heinrich Schuchardt wrote:
> For capsule updates we need to identify the EFI system partition.

Right, but

> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2:
> 	no change
> ---
>  include/efi_loader.h      |  7 +++++++
>  lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 3f2792892f..4a45033476 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -45,6 +45,13 @@ static inline void *guidcpy(void *dst, const void *src)
>  /* Root node */
>  extern efi_handle_t efi_root;
> 
> +/* EFI system partition */
> +extern struct efi_system_partition {
> +	enum if_type if_type;
> +	int devnum;
> +	u8 part;
> +} efi_system_partition;
> +
>  int __efi_entry_check(void);
>  int __efi_exit_check(void);
>  const char *__efi_nesting(void);
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index fc0682bc48..2f752a5e99 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -13,6 +13,8 @@
>  #include <part.h>
>  #include <malloc.h>
> 
> +struct efi_system_partition efi_system_partition;
> +
>  const efi_guid_t efi_block_io_guid = EFI_BLOCK_IO_PROTOCOL_GUID;
> 
>  /**
> @@ -372,6 +374,24 @@ static efi_status_t efi_disk_add_dev(
>  	diskobj->ops.media = &diskobj->media;
>  	if (disk)
>  		*disk = diskobj;
> +
> +	/* Store first EFI system partition */

I don't think that the policy, first comes first serves as system
partition, is a right decision as
- the order of device probe on U-Boot is indeterministic, and
- there can be several partitions that hold a system partition bit.
  You may have OS installed on eMMC, but also may have bootable DVD
  on the system.

-Takahiro Akashi

> +	if (part && !efi_system_partition.if_type) {
> +		int r;
> +		disk_partition_t info;
> +
> +		r = part_get_info(desc, part, &info);
> +		if (r)
> +			return EFI_DEVICE_ERROR;
> +		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
> +			efi_system_partition.if_type = desc->if_type;
> +			efi_system_partition.devnum = desc->devnum;
> +			efi_system_partition.part = part;
> +			EFI_PRINT("EFI system partition: %s %d:%d\n",
> +				  blk_get_if_type_name(desc->if_type),
> +				  desc->devnum, part);
> +		}
> +	}
>  	return EFI_SUCCESS;
>  }
> 
> --
> 2.25.1
> 

[PATCH v2 2/2] efi_loader: identify EFI system partition

[Heinrich Schuchardt]

On 4/6/20 6:21 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> On Sun, Apr 05, 2020 at 11:28:18AM +0200, Heinrich Schuchardt wrote:
>> For capsule updates we need to identify the EFI system partition.
>
> Right, but
>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v2:
>> 	no change
>> ---
>>  include/efi_loader.h      |  7 +++++++
>>  lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
>>  2 files changed, 27 insertions(+)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 3f2792892f..4a45033476 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -45,6 +45,13 @@ static inline void *guidcpy(void *dst, const void *src)
>>  /* Root node */
>>  extern efi_handle_t efi_root;
>>
>> +/* EFI system partition */
>> +extern struct efi_system_partition {
>> +	enum if_type if_type;
>> +	int devnum;
>> +	u8 part;
>> +} efi_system_partition;
>> +
>>  int __efi_entry_check(void);
>>  int __efi_exit_check(void);
>>  const char *__efi_nesting(void);
>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>> index fc0682bc48..2f752a5e99 100644
>> --- a/lib/efi_loader/efi_disk.c
>> +++ b/lib/efi_loader/efi_disk.c
>> @@ -13,6 +13,8 @@
>>  #include <part.h>
>>  #include <malloc.h>
>>
>> +struct efi_system_partition efi_system_partition;
>> +
>>  const efi_guid_t efi_block_io_guid = EFI_BLOCK_IO_PROTOCOL_GUID;
>>
>>  /**
>> @@ -372,6 +374,24 @@ static efi_status_t efi_disk_add_dev(
>>  	diskobj->ops.media = &diskobj->media;
>>  	if (disk)
>>  		*disk = diskobj;
>> +
>> +	/* Store first EFI system partition */
>
> I don't think that the policy, first comes first serves as system
> partition, is a right decision as
> - the order of device probe on U-Boot is indeterministic, and

Indeterministic would mean that on two runs with the same media provided
you will get different results. I cannot see any source for such
randomness in the U-Boot code. In dm_init_and_scan() the device tree is
scanned and drivers and bound in the sequence of occurrence in the
device tree.

> - there can be several partitions that hold a system partition bit.
>   You may have OS installed on eMMC, but also may have bootable DVD
>   on the system.

This is a similar logic like finding the relevant boot.scr script to run.

What would be the alternative?

Definition via Kconfig would mean that a Linux distribution like Debian
would have to provide a separate U-Boot build for each boot medium that
a user might possibly use (eMMC, SD-card, USB, NVME, SCSI).

Best regards

Heinrich

>
> -Takahiro Akashi
>
>> +	if (part && !efi_system_partition.if_type) {
>> +		int r;
>> +		disk_partition_t info;
>> +
>> +		r = part_get_info(desc, part, &info);
>> +		if (r)
>> +			return EFI_DEVICE_ERROR;
>> +		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
>> +			efi_system_partition.if_type = desc->if_type;
>> +			efi_system_partition.devnum = desc->devnum;
>> +			efi_system_partition.part = part;
>> +			EFI_PRINT("EFI system partition: %s %d:%d\n",
>> +				  blk_get_if_type_name(desc->if_type),
>> +				  desc->devnum, part);
>> +		}
>> +	}
>>  	return EFI_SUCCESS;
>>  }
>>
>> --
>> 2.25.1
>>

[PATCH v2 2/2] efi_loader: identify EFI system partition

[AKASHI Takahiro]

On Mon, Apr 06, 2020 at 07:12:56AM +0200, Heinrich Schuchardt wrote:
> On 4/6/20 6:21 AM, AKASHI Takahiro wrote:
> > Heinrich,
> >
> > On Sun, Apr 05, 2020 at 11:28:18AM +0200, Heinrich Schuchardt wrote:
> >> For capsule updates we need to identify the EFI system partition.
> >
> > Right, but
> >
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >> v2:
> >> 	no change
> >> ---
> >>  include/efi_loader.h      |  7 +++++++
> >>  lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
> >>  2 files changed, 27 insertions(+)
> >>
> >> diff --git a/include/efi_loader.h b/include/efi_loader.h
> >> index 3f2792892f..4a45033476 100644
> >> --- a/include/efi_loader.h
> >> +++ b/include/efi_loader.h
> >> @@ -45,6 +45,13 @@ static inline void *guidcpy(void *dst, const void *src)
> >>  /* Root node */
> >>  extern efi_handle_t efi_root;
> >>
> >> +/* EFI system partition */
> >> +extern struct efi_system_partition {
> >> +	enum if_type if_type;
> >> +	int devnum;
> >> +	u8 part;
> >> +} efi_system_partition;
> >> +
> >>  int __efi_entry_check(void);
> >>  int __efi_exit_check(void);
> >>  const char *__efi_nesting(void);
> >> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> >> index fc0682bc48..2f752a5e99 100644
> >> --- a/lib/efi_loader/efi_disk.c
> >> +++ b/lib/efi_loader/efi_disk.c
> >> @@ -13,6 +13,8 @@
> >>  #include <part.h>
> >>  #include <malloc.h>
> >>
> >> +struct efi_system_partition efi_system_partition;
> >> +
> >>  const efi_guid_t efi_block_io_guid = EFI_BLOCK_IO_PROTOCOL_GUID;
> >>
> >>  /**
> >> @@ -372,6 +374,24 @@ static efi_status_t efi_disk_add_dev(
> >>  	diskobj->ops.media = &diskobj->media;
> >>  	if (disk)
> >>  		*disk = diskobj;
> >> +
> >> +	/* Store first EFI system partition */
> >
> > I don't think that the policy, first comes first serves as system
> > partition, is a right decision as
> > - the order of device probe on U-Boot is indeterministic, and
> 
> Indeterministic would mean that on two runs with the same media provided
> you will get different results. I cannot see any source for such
> randomness in the U-Boot code. In dm_init_and_scan() the device tree is
> scanned and drivers and bound in the sequence of occurrence in the
> device tree.
> 
> > - there can be several partitions that hold a system partition bit.
> >   You may have OS installed on eMMC, but also may have bootable DVD
> >   on the system.
> 
> This is a similar logic like finding the relevant boot.scr script to run.
> 
> What would be the alternative?

I think that most UEFI systems have ability for user to specify
"boot order."

-Takahiro Akashi
> 
> Definition via Kconfig would mean that a Linux distribution like Debian
> would have to provide a separate U-Boot build for each boot medium that
> a user might possibly use (eMMC, SD-card, USB, NVME, SCSI).
> 
> Best regards
> 
> Heinrich
> 
> >
> > -Takahiro Akashi
> >
> >> +	if (part && !efi_system_partition.if_type) {
> >> +		int r;
> >> +		disk_partition_t info;
> >> +
> >> +		r = part_get_info(desc, part, &info);
> >> +		if (r)
> >> +			return EFI_DEVICE_ERROR;
> >> +		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
> >> +			efi_system_partition.if_type = desc->if_type;
> >> +			efi_system_partition.devnum = desc->devnum;
> >> +			efi_system_partition.part = part;
> >> +			EFI_PRINT("EFI system partition: %s %d:%d\n",
> >> +				  blk_get_if_type_name(desc->if_type),
> >> +				  desc->devnum, part);
> >> +		}
> >> +	}
> >>  	return EFI_SUCCESS;
> >>  }
> >>
> >> --
> >> 2.25.1
> >>
> 

[PATCH v2 2/2] efi_loader: identify EFI system partition

[AKASHI Takahiro]

Heinrich,

On Mon, Apr 06, 2020 at 02:31:35PM +0900, AKASHI Takahiro wrote:
> On Mon, Apr 06, 2020 at 07:12:56AM +0200, Heinrich Schuchardt wrote:
> > On 4/6/20 6:21 AM, AKASHI Takahiro wrote:
> > > Heinrich,
> > >
> > > On Sun, Apr 05, 2020 at 11:28:18AM +0200, Heinrich Schuchardt wrote:
> > >> For capsule updates we need to identify the EFI system partition.
> > >
> > > Right, but
> > >
> > >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > >> ---
> > >> v2:
> > >> 	no change
> > >> ---
> > >>  include/efi_loader.h      |  7 +++++++
> > >>  lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
> > >>  2 files changed, 27 insertions(+)
> > >>
> > >> diff --git a/include/efi_loader.h b/include/efi_loader.h
> > >> index 3f2792892f..4a45033476 100644
> > >> --- a/include/efi_loader.h
> > >> +++ b/include/efi_loader.h
> > >> @@ -45,6 +45,13 @@ static inline void *guidcpy(void *dst, const void *src)
> > >>  /* Root node */
> > >>  extern efi_handle_t efi_root;
> > >>
> > >> +/* EFI system partition */
> > >> +extern struct efi_system_partition {
> > >> +	enum if_type if_type;
> > >> +	int devnum;
> > >> +	u8 part;
> > >> +} efi_system_partition;
> > >> +
> > >>  int __efi_entry_check(void);
> > >>  int __efi_exit_check(void);
> > >>  const char *__efi_nesting(void);
> > >> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > >> index fc0682bc48..2f752a5e99 100644
> > >> --- a/lib/efi_loader/efi_disk.c
> > >> +++ b/lib/efi_loader/efi_disk.c
> > >> @@ -13,6 +13,8 @@
> > >>  #include <part.h>
> > >>  #include <malloc.h>
> > >>
> > >> +struct efi_system_partition efi_system_partition;
> > >> +
> > >>  const efi_guid_t efi_block_io_guid = EFI_BLOCK_IO_PROTOCOL_GUID;
> > >>
> > >>  /**
> > >> @@ -372,6 +374,24 @@ static efi_status_t efi_disk_add_dev(
> > >>  	diskobj->ops.media = &diskobj->media;
> > >>  	if (disk)
> > >>  		*disk = diskobj;
> > >> +
> > >> +	/* Store first EFI system partition */
> > >
> > > I don't think that the policy, first comes first serves as system
> > > partition, is a right decision as
> > > - the order of device probe on U-Boot is indeterministic, and
> > 
> > Indeterministic would mean that on two runs with the same media provided
> > you will get different results. I cannot see any source for such
> > randomness in the U-Boot code. In dm_init_and_scan() the device tree is
> > scanned and drivers and bound in the sequence of occurrence in the
> > device tree.
> > 
> > > - there can be several partitions that hold a system partition bit.
> > >   You may have OS installed on eMMC, but also may have bootable DVD
> > >   on the system.
> > 
> > This is a similar logic like finding the relevant boot.scr script to run.
> > 
> > What would be the alternative?
> 
> I think that most UEFI systems have ability for user to specify
> "boot order."

Any comment?
The discussion and your patch will have some impact on
my efi capsule patch.

-Takahiro Akashi

> -Takahiro Akashi
> > 
> > Definition via Kconfig would mean that a Linux distribution like Debian
> > would have to provide a separate U-Boot build for each boot medium that
> > a user might possibly use (eMMC, SD-card, USB, NVME, SCSI).
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > >
> > > -Takahiro Akashi
> > >
> > >> +	if (part && !efi_system_partition.if_type) {
> > >> +		int r;
> > >> +		disk_partition_t info;
> > >> +
> > >> +		r = part_get_info(desc, part, &info);
> > >> +		if (r)
> > >> +			return EFI_DEVICE_ERROR;
> > >> +		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
> > >> +			efi_system_partition.if_type = desc->if_type;
> > >> +			efi_system_partition.devnum = desc->devnum;
> > >> +			efi_system_partition.part = part;
> > >> +			EFI_PRINT("EFI system partition: %s %d:%d\n",
> > >> +				  blk_get_if_type_name(desc->if_type),
> > >> +				  desc->devnum, part);
> > >> +		}
> > >> +	}
> > >>  	return EFI_SUCCESS;
> > >>  }
> > >>
> > >> --
> > >> 2.25.1
> > >>
> > 

[PATCH v2 1/2] part: detect EFI system partition

[AKASHI Takahiro]

Heinrich,

On Sun, Apr 05, 2020 at 11:28:17AM +0200, Heinrich Schuchardt wrote:
> Up to now for MBR and GPT partitions the info field 'bootable' was set to 1
> if either the partition was an EFI system partition or the bootable flag
> was set.
> 
> Turn info field 'bootable' into a bit mask with separate bits for bootable
> and EFI system partition.
> 
> This will allow us to identify the EFI system partition in the UEFI
> sub-system.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2:
> 	used BIT() macro to define bit mask
> ---
>  disk/part_dos.c | 10 ++++++++--
>  disk/part_efi.c | 12 ++++++++----
>  include/part.h  | 11 ++++++++++-
>  3 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/disk/part_dos.c b/disk/part_dos.c
> index 83ff40d310..0ec7f1628e 100644
> --- a/disk/part_dos.c
> +++ b/disk/part_dos.c
> @@ -45,9 +45,15 @@ static inline int is_extended(int part_type)
>  	    part_type == 0x85);
>  }
> 
> -static inline int is_bootable(dos_partition_t *p)
> +static int is_bootable(dos_partition_t *p)
>  {
> -	return (p->sys_ind == 0xef) || (p->boot_ind == 0x80);
> +	int ret = 0;
> +
> +	if (p->sys_ind == 0xef)
> +		ret |= PART_EFI_SYSTEM_PARTITION;
> +	if (p->boot_ind == 0x80)
> +		ret |= PART_BOOTABLE;
> +	return ret;
>  }

The return value is no longer boolean, so the function's name
should be changed to avoid confusion. Say, get_bootable_flags()?
Then  another function, or inline function should be provided.

bool part_is_bootable(blk_desc *bdev)

for checking if the device is "bootable."

>  static void print_one_part(dos_partition_t *p, lbaint_t ext_part_sector,
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index b2e157d9c1..19f1f43f4e 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -71,11 +71,15 @@ static char *print_efiname(gpt_entry *pte)
> 
>  static const efi_guid_t system_guid = PARTITION_SYSTEM_GUID;
> 
> -static inline int is_bootable(gpt_entry *p)
> +static int is_bootable(gpt_entry *p)
>  {
> -	return p->attributes.fields.legacy_bios_bootable ||
> -		!memcmp(&(p->partition_type_guid), &system_guid,
> -			sizeof(efi_guid_t));
> +	int ret = 0;
> +
> +	if (!memcmp(&p->partition_type_guid, &system_guid, sizeof(efi_guid_t)))
> +		ret |=  PART_EFI_SYSTEM_PARTITION;
> +	if (p->attributes.fields.legacy_bios_bootable)
> +		ret |=  PART_BOOTABLE;
> +	return ret;
>  }

ditto, and again

bool part_is_efi_system()

-Takahiro Akashi

>  static int validate_gpt_header(gpt_header *gpt_h, lbaint_t lba,
> diff --git a/include/part.h b/include/part.h
> index 0b5cf3d5e8..f3442ef85d 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -51,13 +51,22 @@ struct block_drvr {
>  #define PART_TYPE_LEN 32
>  #define MAX_SEARCH_PARTITIONS 64
> 
> +#define PART_BOOTABLE			BIT(0)
> +#define PART_EFI_SYSTEM_PARTITION	BIT(1)
> +
>  typedef struct disk_partition {
>  	lbaint_t	start;	/* # of first block in partition	*/
>  	lbaint_t	size;	/* number of blocks in partition	*/
>  	ulong	blksz;		/* block size in bytes			*/
>  	uchar	name[PART_NAME_LEN];	/* partition name			*/
>  	uchar	type[PART_TYPE_LEN];	/* string type description		*/
> -	int	bootable;	/* Active/Bootable flag is set		*/
> +	/*
> +	 * The bootable is a bitmask with the following fields:
> +	 *
> +	 * PART_BOOTABLE		the MBR bootable flag is set
> +	 * PART_EFI_SYSTEM_PARTITION	the partition is an EFI system partition
> +	 */
> +	int	bootable;
>  #if CONFIG_IS_ENABLED(PARTITION_UUIDS)
>  	char	uuid[UUID_STR_LEN + 1];	/* filesystem UUID as string, if exists	*/
>  #endif
> --
> 2.25.1
> 

[PATCH v2 2/2] efi_loader: identify EFI system partition

[Heinrich Schuchardt]

Am April 14, 2020 5:20:38 AM UTC schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>Heinrich,
>
>On Mon, Apr 06, 2020 at 02:31:35PM +0900, AKASHI Takahiro wrote:
>> On Mon, Apr 06, 2020 at 07:12:56AM +0200, Heinrich Schuchardt wrote:
>> > On 4/6/20 6:21 AM, AKASHI Takahiro wrote:
>> > > Heinrich,
>> > >
>> > > On Sun, Apr 05, 2020 at 11:28:18AM +0200, Heinrich Schuchardt
>wrote:
>> > >> For capsule updates we need to identify the EFI system
>partition.
>> > >
>> > > Right, but
>> > >
>> > >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> > >> ---
>> > >> v2:
>> > >> 	no change
>> > >> ---
>> > >>  include/efi_loader.h      |  7 +++++++
>> > >>  lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
>> > >>  2 files changed, 27 insertions(+)
>> > >>
>> > >> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> > >> index 3f2792892f..4a45033476 100644
>> > >> --- a/include/efi_loader.h
>> > >> +++ b/include/efi_loader.h
>> > >> @@ -45,6 +45,13 @@ static inline void *guidcpy(void *dst, const
>void *src)
>> > >>  /* Root node */
>> > >>  extern efi_handle_t efi_root;
>> > >>
>> > >> +/* EFI system partition */
>> > >> +extern struct efi_system_partition {
>> > >> +	enum if_type if_type;
>> > >> +	int devnum;
>> > >> +	u8 part;
>> > >> +} efi_system_partition;
>> > >> +
>> > >>  int __efi_entry_check(void);
>> > >>  int __efi_exit_check(void);
>> > >>  const char *__efi_nesting(void);
>> > >> diff --git a/lib/efi_loader/efi_disk.c
>b/lib/efi_loader/efi_disk.c
>> > >> index fc0682bc48..2f752a5e99 100644
>> > >> --- a/lib/efi_loader/efi_disk.c
>> > >> +++ b/lib/efi_loader/efi_disk.c
>> > >> @@ -13,6 +13,8 @@
>> > >>  #include <part.h>
>> > >>  #include <malloc.h>
>> > >>
>> > >> +struct efi_system_partition efi_system_partition;
>> > >> +
>> > >>  const efi_guid_t efi_block_io_guid =
>EFI_BLOCK_IO_PROTOCOL_GUID;
>> > >>
>> > >>  /**
>> > >> @@ -372,6 +374,24 @@ static efi_status_t efi_disk_add_dev(
>> > >>  	diskobj->ops.media = &diskobj->media;
>> > >>  	if (disk)
>> > >>  		*disk = diskobj;
>> > >> +
>> > >> +	/* Store first EFI system partition */
>> > >
>> > > I don't think that the policy, first comes first serves as system
>> > > partition, is a right decision as
>> > > - the order of device probe on U-Boot is indeterministic, and
>> > 
>> > Indeterministic would mean that on two runs with the same media
>provided
>> > you will get different results. I cannot see any source for such
>> > randomness in the U-Boot code. In dm_init_and_scan() the device
>tree is
>> > scanned and drivers and bound in the sequence of occurrence in the
>> > device tree.
>> > 
>> > > - there can be several partitions that hold a system partition
>bit.
>> > >   You may have OS installed on eMMC, but also may have bootable
>DVD
>> > >   on the system.
>> > 
>> > This is a similar logic like finding the relevant boot.scr script
>to run.
>> > 
>> > What would be the alternative?
>> 
>> I think that most UEFI systems have ability for user to specify
>> "boot order."
>
>Any comment?
>The discussion and your patch will have some impact on
>my efi capsule patch.

Concerning capsules the spec says we should use the boot device. So my patch doesn't help you there.

For the storage of variables I still need this patch. I will adjust the commit message.

Best regards

Heinrich

>
>-Takahiro Akashi
>
>> -Takahiro Akashi
>> > 
>> > Definition via Kconfig would mean that a Linux distribution like
>Debian
>> > would have to provide a separate U-Boot build for each boot medium
>that
>> > a user might possibly use (eMMC, SD-card, USB, NVME, SCSI).
>> > 
>> > Best regards
>> > 
>> > Heinrich
>> > 
>> > >
>> > > -Takahiro Akashi
>> > >
>> > >> +	if (part && !efi_system_partition.if_type) {
>> > >> +		int r;
>> > >> +		disk_partition_t info;
>> > >> +
>> > >> +		r = part_get_info(desc, part, &info);
>> > >> +		if (r)
>> > >> +			return EFI_DEVICE_ERROR;
>> > >> +		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
>> > >> +			efi_system_partition.if_type = desc->if_type;
>> > >> +			efi_system_partition.devnum = desc->devnum;
>> > >> +			efi_system_partition.part = part;
>> > >> +			EFI_PRINT("EFI system partition: %s %d:%d\n",
>> > >> +				  blk_get_if_type_name(desc->if_type),
>> > >> +				  desc->devnum, part);
>> > >> +		}
>> > >> +	}
>> > >>  	return EFI_SUCCESS;
>> > >>  }
>> > >>
>> > >> --
>> > >> 2.25.1
>> > >>
>> > 

[PATCH v2 1/2] part: detect EFI system partition

[Heinrich Schuchardt]

Am April 14, 2020 5:31:25 AM UTC schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>Heinrich,
>
>On Sun, Apr 05, 2020 at 11:28:17AM +0200, Heinrich Schuchardt wrote:
>> Up to now for MBR and GPT partitions the info field 'bootable' was
>set to 1
>> if either the partition was an EFI system partition or the bootable
>flag
>> was set.
>> 
>> Turn info field 'bootable' into a bit mask with separate bits for
>bootable
>> and EFI system partition.
>> 
>> This will allow us to identify the EFI system partition in the UEFI
>> sub-system.
>> 
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v2:
>> 	used BIT() macro to define bit mask
>> ---
>>  disk/part_dos.c | 10 ++++++++--
>>  disk/part_efi.c | 12 ++++++++----
>>  include/part.h  | 11 ++++++++++-
>>  3 files changed, 26 insertions(+), 7 deletions(-)
>> 
>> diff --git a/disk/part_dos.c b/disk/part_dos.c
>> index 83ff40d310..0ec7f1628e 100644
>> --- a/disk/part_dos.c
>> +++ b/disk/part_dos.c
>> @@ -45,9 +45,15 @@ static inline int is_extended(int part_type)
>>  	    part_type == 0x85);
>>  }
>> 
>> -static inline int is_bootable(dos_partition_t *p)
>> +static int is_bootable(dos_partition_t *p)
>>  {
>> -	return (p->sys_ind == 0xef) || (p->boot_ind == 0x80);
>> +	int ret = 0;
>> +
>> +	if (p->sys_ind == 0xef)
>> +		ret |= PART_EFI_SYSTEM_PARTITION;
>> +	if (p->boot_ind == 0x80)
>> +		ret |= PART_BOOTABLE;
>> +	return ret;
>>  }
>
>The return value is no longer boolean, so the function's name
>should be changed to avoid confusion. Say, get_bootable_flags()?
>Then  another function, or inline function should be provided.
>
>bool part_is_bootable(blk_desc *bdev)
>
>for checking if the device is "bootable."
>

Logical operators 'if', '!', '&&', '||' regard any non-zero value as true. So I see no benefit of such a second function.

Best regards

Heinrich

>>  static void print_one_part(dos_partition_t *p, lbaint_t
>ext_part_sector,
>> diff --git a/disk/part_efi.c b/disk/part_efi.c
>> index b2e157d9c1..19f1f43f4e 100644
>> --- a/disk/part_efi.c
>> +++ b/disk/part_efi.c
>> @@ -71,11 +71,15 @@ static char *print_efiname(gpt_entry *pte)
>> 
>>  static const efi_guid_t system_guid = PARTITION_SYSTEM_GUID;
>> 
>> -static inline int is_bootable(gpt_entry *p)
>> +static int is_bootable(gpt_entry *p)
>>  {
>> -	return p->attributes.fields.legacy_bios_bootable ||
>> -		!memcmp(&(p->partition_type_guid), &system_guid,
>> -			sizeof(efi_guid_t));
>> +	int ret = 0;
>> +
>> +	if (!memcmp(&p->partition_type_guid, &system_guid,
>sizeof(efi_guid_t)))
>> +		ret |=  PART_EFI_SYSTEM_PARTITION;
>> +	if (p->attributes.fields.legacy_bios_bootable)
>> +		ret |=  PART_BOOTABLE;
>> +	return ret;
>>  }
>
>ditto, and again
>
>bool part_is_efi_system()
>
>-Takahiro Akashi
>
>>  static int validate_gpt_header(gpt_header *gpt_h, lbaint_t lba,
>> diff --git a/include/part.h b/include/part.h
>> index 0b5cf3d5e8..f3442ef85d 100644
>> --- a/include/part.h
>> +++ b/include/part.h
>> @@ -51,13 +51,22 @@ struct block_drvr {
>>  #define PART_TYPE_LEN 32
>>  #define MAX_SEARCH_PARTITIONS 64
>> 
>> +#define PART_BOOTABLE			BIT(0)
>> +#define PART_EFI_SYSTEM_PARTITION	BIT(1)
>> +
>>  typedef struct disk_partition {
>>  	lbaint_t	start;	/* # of first block in partition	*/
>>  	lbaint_t	size;	/* number of blocks in partition	*/
>>  	ulong	blksz;		/* block size in bytes			*/
>>  	uchar	name[PART_NAME_LEN];	/* partition name			*/
>>  	uchar	type[PART_TYPE_LEN];	/* string type description		*/
>> -	int	bootable;	/* Active/Bootable flag is set		*/
>> +	/*
>> +	 * The bootable is a bitmask with the following fields:
>> +	 *
>> +	 * PART_BOOTABLE		the MBR bootable flag is set
>> +	 * PART_EFI_SYSTEM_PARTITION	the partition is an EFI system
>partition
>> +	 */
>> +	int	bootable;
>>  #if CONFIG_IS_ENABLED(PARTITION_UUIDS)
>>  	char	uuid[UUID_STR_LEN + 1];	/* filesystem UUID as string, if
>exists	*/
>>  #endif
>> --
>> 2.25.1
>> 

[PATCH v2 2/2] efi_loader: identify EFI system partition

[AKASHI Takahiro]

On Tue, Apr 14, 2020 at 05:53:43AM +0000, Heinrich Schuchardt wrote:
> Am April 14, 2020 5:20:38 AM UTC schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
> >Heinrich,
> >
> >On Mon, Apr 06, 2020 at 02:31:35PM +0900, AKASHI Takahiro wrote:
> >> On Mon, Apr 06, 2020 at 07:12:56AM +0200, Heinrich Schuchardt wrote:
> >> > On 4/6/20 6:21 AM, AKASHI Takahiro wrote:
> >> > > Heinrich,
> >> > >
> >> > > On Sun, Apr 05, 2020 at 11:28:18AM +0200, Heinrich Schuchardt
> >wrote:
> >> > >> For capsule updates we need to identify the EFI system
> >partition.
> >> > >
> >> > > Right, but
> >> > >
> >> > >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> > >> ---
> >> > >> v2:
> >> > >> 	no change
> >> > >> ---
> >> > >>  include/efi_loader.h      |  7 +++++++
> >> > >>  lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
> >> > >>  2 files changed, 27 insertions(+)
> >> > >>
> >> > >> diff --git a/include/efi_loader.h b/include/efi_loader.h
> >> > >> index 3f2792892f..4a45033476 100644
> >> > >> --- a/include/efi_loader.h
> >> > >> +++ b/include/efi_loader.h
> >> > >> @@ -45,6 +45,13 @@ static inline void *guidcpy(void *dst, const
> >void *src)
> >> > >>  /* Root node */
> >> > >>  extern efi_handle_t efi_root;
> >> > >>
> >> > >> +/* EFI system partition */
> >> > >> +extern struct efi_system_partition {
> >> > >> +	enum if_type if_type;
> >> > >> +	int devnum;
> >> > >> +	u8 part;
> >> > >> +} efi_system_partition;
> >> > >> +
> >> > >>  int __efi_entry_check(void);
> >> > >>  int __efi_exit_check(void);
> >> > >>  const char *__efi_nesting(void);
> >> > >> diff --git a/lib/efi_loader/efi_disk.c
> >b/lib/efi_loader/efi_disk.c
> >> > >> index fc0682bc48..2f752a5e99 100644
> >> > >> --- a/lib/efi_loader/efi_disk.c
> >> > >> +++ b/lib/efi_loader/efi_disk.c
> >> > >> @@ -13,6 +13,8 @@
> >> > >>  #include <part.h>
> >> > >>  #include <malloc.h>
> >> > >>
> >> > >> +struct efi_system_partition efi_system_partition;
> >> > >> +
> >> > >>  const efi_guid_t efi_block_io_guid =
> >EFI_BLOCK_IO_PROTOCOL_GUID;
> >> > >>
> >> > >>  /**
> >> > >> @@ -372,6 +374,24 @@ static efi_status_t efi_disk_add_dev(
> >> > >>  	diskobj->ops.media = &diskobj->media;
> >> > >>  	if (disk)
> >> > >>  		*disk = diskobj;
> >> > >> +
> >> > >> +	/* Store first EFI system partition */
> >> > >
> >> > > I don't think that the policy, first comes first serves as system
> >> > > partition, is a right decision as
> >> > > - the order of device probe on U-Boot is indeterministic, and
> >> > 
> >> > Indeterministic would mean that on two runs with the same media
> >provided
> >> > you will get different results. I cannot see any source for such
> >> > randomness in the U-Boot code. In dm_init_and_scan() the device
> >tree is
> >> > scanned and drivers and bound in the sequence of occurrence in the
> >> > device tree.
> >> > 
> >> > > - there can be several partitions that hold a system partition
> >bit.
> >> > >   You may have OS installed on eMMC, but also may have bootable
> >DVD
> >> > >   on the system.
> >> > 
> >> > This is a similar logic like finding the relevant boot.scr script
> >to run.
> >> > 
> >> > What would be the alternative?
> >> 
> >> I think that most UEFI systems have ability for user to specify
> >> "boot order."
> >
> >Any comment?
> >The discussion and your patch will have some impact on
> >my efi capsule patch.
> 
> Concerning capsules the spec says we should use the boot device. So my patch doesn't help you there.

Your commit message says,
  "For capsule updates we need to identify the EFI system partition."

and then I made some counter comment.
So now you agreed with my comment, don't you?
(I need to confirm this to work on capsule patch.)

> For the storage of variables I still need this patch. I will adjust the commit message.

Even in this case, I believe that the first device detected in your logic
is not always a "valid" device for your purpose.

-Takahiro Akashi

> 
> Best regards
> 
> Heinrich
> 
> >
> >-Takahiro Akashi
> >
> >> -Takahiro Akashi
> >> > 
> >> > Definition via Kconfig would mean that a Linux distribution like
> >Debian
> >> > would have to provide a separate U-Boot build for each boot medium
> >that
> >> > a user might possibly use (eMMC, SD-card, USB, NVME, SCSI).
> >> > 
> >> > Best regards
> >> > 
> >> > Heinrich
> >> > 
> >> > >
> >> > > -Takahiro Akashi
> >> > >
> >> > >> +	if (part && !efi_system_partition.if_type) {
> >> > >> +		int r;
> >> > >> +		disk_partition_t info;
> >> > >> +
> >> > >> +		r = part_get_info(desc, part, &info);
> >> > >> +		if (r)
> >> > >> +			return EFI_DEVICE_ERROR;
> >> > >> +		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
> >> > >> +			efi_system_partition.if_type = desc->if_type;
> >> > >> +			efi_system_partition.devnum = desc->devnum;
> >> > >> +			efi_system_partition.part = part;
> >> > >> +			EFI_PRINT("EFI system partition: %s %d:%d\n",
> >> > >> +				  blk_get_if_type_name(desc->if_type),
> >> > >> +				  desc->devnum, part);
> >> > >> +		}
> >> > >> +	}
> >> > >>  	return EFI_SUCCESS;
> >> > >>  }
> >> > >>
> >> > >> --
> >> > >> 2.25.1
> >> > >>
> >> > 
> 

[PATCH v2 2/2] efi_loader: identify EFI system partition

[Heinrich Schuchardt]

On 2020-04-14 08:12, AKASHI Takahiro wrote:
> On Tue, Apr 14, 2020 at 05:53:43AM +0000, Heinrich Schuchardt wrote:
>> Am April 14, 2020 5:20:38 AM UTC schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>>> Heinrich,
>>>
>>> On Mon, Apr 06, 2020 at 02:31:35PM +0900, AKASHI Takahiro wrote:
>>>> On Mon, Apr 06, 2020 at 07:12:56AM +0200, Heinrich Schuchardt wrote:
>>>>> On 4/6/20 6:21 AM, AKASHI Takahiro wrote:
>>>>>> Heinrich,
>>>>>>
>>>>>> On Sun, Apr 05, 2020 at 11:28:18AM +0200, Heinrich Schuchardt
>>> wrote:
>>>>>>> For capsule updates we need to identify the EFI system
>>> partition.
>>>>>>
>>>>>> Right, but
>>>>>>
>>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> 	no change
>>>>>>> ---
>>>>>>>  include/efi_loader.h      |  7 +++++++
>>>>>>>  lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
>>>>>>>  2 files changed, 27 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>>>>> index 3f2792892f..4a45033476 100644
>>>>>>> --- a/include/efi_loader.h
>>>>>>> +++ b/include/efi_loader.h
>>>>>>> @@ -45,6 +45,13 @@ static inline void *guidcpy(void *dst, const
>>> void *src)
>>>>>>>  /* Root node */
>>>>>>>  extern efi_handle_t efi_root;
>>>>>>>
>>>>>>> +/* EFI system partition */
>>>>>>> +extern struct efi_system_partition {
>>>>>>> +	enum if_type if_type;
>>>>>>> +	int devnum;
>>>>>>> +	u8 part;
>>>>>>> +} efi_system_partition;
>>>>>>> +
>>>>>>>  int __efi_entry_check(void);
>>>>>>>  int __efi_exit_check(void);
>>>>>>>  const char *__efi_nesting(void);
>>>>>>> diff --git a/lib/efi_loader/efi_disk.c
>>> b/lib/efi_loader/efi_disk.c
>>>>>>> index fc0682bc48..2f752a5e99 100644
>>>>>>> --- a/lib/efi_loader/efi_disk.c
>>>>>>> +++ b/lib/efi_loader/efi_disk.c
>>>>>>> @@ -13,6 +13,8 @@
>>>>>>>  #include <part.h>
>>>>>>>  #include <malloc.h>
>>>>>>>
>>>>>>> +struct efi_system_partition efi_system_partition;
>>>>>>> +
>>>>>>>  const efi_guid_t efi_block_io_guid =
>>> EFI_BLOCK_IO_PROTOCOL_GUID;
>>>>>>>
>>>>>>>  /**
>>>>>>> @@ -372,6 +374,24 @@ static efi_status_t efi_disk_add_dev(
>>>>>>>  	diskobj->ops.media = &diskobj->media;
>>>>>>>  	if (disk)
>>>>>>>  		*disk = diskobj;
>>>>>>> +
>>>>>>> +	/* Store first EFI system partition */
>>>>>>
>>>>>> I don't think that the policy, first comes first serves as system
>>>>>> partition, is a right decision as
>>>>>> - the order of device probe on U-Boot is indeterministic, and
>>>>>
>>>>> Indeterministic would mean that on two runs with the same media
>>> provided
>>>>> you will get different results. I cannot see any source for such
>>>>> randomness in the U-Boot code. In dm_init_and_scan() the device
>>> tree is
>>>>> scanned and drivers and bound in the sequence of occurrence in the
>>>>> device tree.
>>>>>
>>>>>> - there can be several partitions that hold a system partition
>>> bit.
>>>>>>   You may have OS installed on eMMC, but also may have bootable
>>> DVD
>>>>>>   on the system.
>>>>>
>>>>> This is a similar logic like finding the relevant boot.scr script
>>> to run.
>>>>>
>>>>> What would be the alternative?
>>>>
>>>> I think that most UEFI systems have ability for user to specify
>>>> "boot order."
>>>
>>> Any comment?
>>> The discussion and your patch will have some impact on
>>> my efi capsule patch.
>>
>> Concerning capsules the spec says we should use the boot device. So my patch doesn't help you there.
>
> Your commit message says,
>   "For capsule updates we need to identify the EFI system partition."
>
> and then I made some counter comment.
> So now you agreed with my comment, don't you?
> (I need to confirm this to work on capsule patch.)

You can stick to your original design.

>
>> For the storage of variables I still need this patch. I will adjust the commit message.
>
> Even in this case, I believe that the first device detected in your logic
> is not always a "valid" device for your purpose.

Do you have a better suggestion?

Best regards

Heinrich

>
> -Takahiro Akashi
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>> -Takahiro Akashi
>>>>>
>>>>> Definition via Kconfig would mean that a Linux distribution like
>>> Debian
>>>>> would have to provide a separate U-Boot build for each boot medium
>>> that
>>>>> a user might possibly use (eMMC, SD-card, USB, NVME, SCSI).
>>>>>
>>>>> Best regards
>>>>>
>>>>> Heinrich
>>>>>
>>>>>>
>>>>>> -Takahiro Akashi
>>>>>>
>>>>>>> +	if (part && !efi_system_partition.if_type) {
>>>>>>> +		int r;
>>>>>>> +		disk_partition_t info;
>>>>>>> +
>>>>>>> +		r = part_get_info(desc, part, &info);
>>>>>>> +		if (r)
>>>>>>> +			return EFI_DEVICE_ERROR;
>>>>>>> +		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
>>>>>>> +			efi_system_partition.if_type = desc->if_type;
>>>>>>> +			efi_system_partition.devnum = desc->devnum;
>>>>>>> +			efi_system_partition.part = part;
>>>>>>> +			EFI_PRINT("EFI system partition: %s %d:%d\n",
>>>>>>> +				  blk_get_if_type_name(desc->if_type),
>>>>>>> +				  desc->devnum, part);
>>>>>>> +		}
>>>>>>> +	}
>>>>>>>  	return EFI_SUCCESS;
>>>>>>>  }
>>>>>>>
>>>>>>> --
>>>>>>> 2.25.1
>>>>>>>
>>>>>
>>

[PATCH v2 2/2] efi_loader: identify EFI system partition

[AKASHI Takahiro]

Heinrich,

On Tue, Apr 14, 2020 at 09:41:51AM +0200, Heinrich Schuchardt wrote:
> On 2020-04-14 08:12, AKASHI Takahiro wrote:
> > On Tue, Apr 14, 2020 at 05:53:43AM +0000, Heinrich Schuchardt wrote:
> >> Am April 14, 2020 5:20:38 AM UTC schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
> >>> Heinrich,
> >>>
> >>> On Mon, Apr 06, 2020 at 02:31:35PM +0900, AKASHI Takahiro wrote:
> >>>> On Mon, Apr 06, 2020 at 07:12:56AM +0200, Heinrich Schuchardt wrote:
> >>>>> On 4/6/20 6:21 AM, AKASHI Takahiro wrote:
> >>>>>> Heinrich,
> >>>>>>
> >>>>>> On Sun, Apr 05, 2020 at 11:28:18AM +0200, Heinrich Schuchardt
> >>> wrote:
> >>>>>>> For capsule updates we need to identify the EFI system
> >>> partition.
> >>>>>>
> >>>>>> Right, but
> >>>>>>
> >>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>> ---
> >>>>>>> v2:
> >>>>>>> 	no change
> >>>>>>> ---
> >>>>>>>  include/efi_loader.h      |  7 +++++++
> >>>>>>>  lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
> >>>>>>>  2 files changed, 27 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>>>>>> index 3f2792892f..4a45033476 100644
> >>>>>>> --- a/include/efi_loader.h
> >>>>>>> +++ b/include/efi_loader.h
> >>>>>>> @@ -45,6 +45,13 @@ static inline void *guidcpy(void *dst, const
> >>> void *src)
> >>>>>>>  /* Root node */
> >>>>>>>  extern efi_handle_t efi_root;
> >>>>>>>
> >>>>>>> +/* EFI system partition */
> >>>>>>> +extern struct efi_system_partition {
> >>>>>>> +	enum if_type if_type;
> >>>>>>> +	int devnum;
> >>>>>>> +	u8 part;
> >>>>>>> +} efi_system_partition;
> >>>>>>> +
> >>>>>>>  int __efi_entry_check(void);
> >>>>>>>  int __efi_exit_check(void);
> >>>>>>>  const char *__efi_nesting(void);
> >>>>>>> diff --git a/lib/efi_loader/efi_disk.c
> >>> b/lib/efi_loader/efi_disk.c
> >>>>>>> index fc0682bc48..2f752a5e99 100644
> >>>>>>> --- a/lib/efi_loader/efi_disk.c
> >>>>>>> +++ b/lib/efi_loader/efi_disk.c
> >>>>>>> @@ -13,6 +13,8 @@
> >>>>>>>  #include <part.h>
> >>>>>>>  #include <malloc.h>
> >>>>>>>
> >>>>>>> +struct efi_system_partition efi_system_partition;
> >>>>>>> +
> >>>>>>>  const efi_guid_t efi_block_io_guid =
> >>> EFI_BLOCK_IO_PROTOCOL_GUID;
> >>>>>>>
> >>>>>>>  /**
> >>>>>>> @@ -372,6 +374,24 @@ static efi_status_t efi_disk_add_dev(
> >>>>>>>  	diskobj->ops.media = &diskobj->media;
> >>>>>>>  	if (disk)
> >>>>>>>  		*disk = diskobj;
> >>>>>>> +
> >>>>>>> +	/* Store first EFI system partition */
> >>>>>>
> >>>>>> I don't think that the policy, first comes first serves as system
> >>>>>> partition, is a right decision as
> >>>>>> - the order of device probe on U-Boot is indeterministic, and
> >>>>>
> >>>>> Indeterministic would mean that on two runs with the same media
> >>> provided
> >>>>> you will get different results. I cannot see any source for such
> >>>>> randomness in the U-Boot code. In dm_init_and_scan() the device
> >>> tree is
> >>>>> scanned and drivers and bound in the sequence of occurrence in the
> >>>>> device tree.
> >>>>>
> >>>>>> - there can be several partitions that hold a system partition
> >>> bit.
> >>>>>>   You may have OS installed on eMMC, but also may have bootable
> >>> DVD
> >>>>>>   on the system.
> >>>>>
> >>>>> This is a similar logic like finding the relevant boot.scr script
> >>> to run.
> >>>>>
> >>>>> What would be the alternative?
> >>>>
> >>>> I think that most UEFI systems have ability for user to specify
> >>>> "boot order."
> >>>
> >>> Any comment?
> >>> The discussion and your patch will have some impact on
> >>> my efi capsule patch.
> >>
> >> Concerning capsules the spec says we should use the boot device. So my patch doesn't help you there.
> >
> > Your commit message says,
> >   "For capsule updates we need to identify the EFI system partition."
> >
> > and then I made some counter comment.
> > So now you agreed with my comment, don't you?
> > (I need to confirm this to work on capsule patch.)
> 
> You can stick to your original design.

Thanks

> >
> >> For the storage of variables I still need this patch. I will adjust the commit message.
> >
> > Even in this case, I believe that the first device detected in your logic
> > is not always a "valid" device for your purpose.
> 
> Do you have a better suggestion?

The root cause would be that there is no notion of "boot device"
in U-Boot. So I would suggest
1) we should add Kconfig option to specify it
just as we do for U-Boot environment (variables), and then
check if the partition has a system partition bit at boot time.
2) you would follow the similar way as I do in capsule support.
or
3) you would first examine what EDK2 does in this respect.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >
> > -Takahiro Akashi
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> -Takahiro Akashi
> >>>
> >>>> -Takahiro Akashi
> >>>>>
> >>>>> Definition via Kconfig would mean that a Linux distribution like
> >>> Debian
> >>>>> would have to provide a separate U-Boot build for each boot medium
> >>> that
> >>>>> a user might possibly use (eMMC, SD-card, USB, NVME, SCSI).
> >>>>>
> >>>>> Best regards
> >>>>>
> >>>>> Heinrich
> >>>>>
> >>>>>>
> >>>>>> -Takahiro Akashi
> >>>>>>
> >>>>>>> +	if (part && !efi_system_partition.if_type) {
> >>>>>>> +		int r;
> >>>>>>> +		disk_partition_t info;
> >>>>>>> +
> >>>>>>> +		r = part_get_info(desc, part, &info);
> >>>>>>> +		if (r)
> >>>>>>> +			return EFI_DEVICE_ERROR;
> >>>>>>> +		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
> >>>>>>> +			efi_system_partition.if_type = desc->if_type;
> >>>>>>> +			efi_system_partition.devnum = desc->devnum;
> >>>>>>> +			efi_system_partition.part = part;
> >>>>>>> +			EFI_PRINT("EFI system partition: %s %d:%d\n",
> >>>>>>> +				  blk_get_if_type_name(desc->if_type),
> >>>>>>> +				  desc->devnum, part);
> >>>>>>> +		}
> >>>>>>> +	}
> >>>>>>>  	return EFI_SUCCESS;
> >>>>>>>  }
> >>>>>>>
> >>>>>>> --
> >>>>>>> 2.25.1
> >>>>>>>
> >>>>>
> >>
>