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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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