[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 at 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 at 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
> >> > >>
> >> > 
>