From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Wed, 15 Apr 2020 07:57:28 +0900 Subject: [PATCH v2 2/2] efi_loader: identify EFI system partition In-Reply-To: References: <20200405092818.6417-1-xypron.glpk@gmx.de> <20200405092818.6417-3-xypron.glpk@gmx.de> <20200406042129.GA27014@linaro.org> <20200406053134.GB27014@linaro.org> <20200414052038.GA3168@laputa> <70BB0251-C4E8-462D-BD78-B84CAC530DBB@gmx.de> <20200414061251.GC3168@laputa> Message-ID: <20200414225728.GA6943@laputa> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 : > >>> 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 > >>>>>>> --- > >>>>>>> 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 > >>>>>>> #include > >>>>>>> > >>>>>>> +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 > >>>>>>> > >>>>> > >> >