[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 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.)

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