Hi Heinrich,
On Tue, 12 Feb 2019 at 17:53, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 2/12/19 8:24 AM, AKASHI Takahiro wrote:
Hi Heinrich, Simon,
On Sat, Feb 09, 2019 at 05:00:33PM -0600, Simon Glass wrote:
Hi Heinrich,
On Fri, 8 Feb 2019 at 03:36, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 2/8/19 9:15 AM, AKASHI Takahiro wrote:
# bootefi doesn't work with this patch set yet
This patch set came from the past discussion[1] on my "removable device support" patch and is intended to be an attempt to integrate efi objects into u-boot's Driver Model as much seamlessly as possible.
[1] https://lists.denx.de/pipermail/u-boot/2019-January/354010.html
Since this patch is a prototype (or POC, Proof-Of-Concept), the aim here is to discuss further about how in a better shape we will be able to merge the two worlds.
After RFC, Simon suggested that efi protocols could be also presented as DM devices. This is a major change in RFC v2.
Hello Takahiro,
thanks a lot for laying out your thoughts about a possible integration of the EFI subsystem and the driver model. Thanks also for providing a first implementation.
Yes indeed. It is very clever what you have been able to do Takahiro.
I think that I'm going to extremes here :)
The other extreme is what EDK2 does. They live with an EFI only model.
I think moving the DM model in this direction would be feasible and would be much more consistent than trying to map EFI objects onto DM structures with different semantics. But I do not see the man power to do such a change.
I think you might be describing some other bootloader :-) I think it's fine to add EFI support to U-Boot but I don't like the idea of moving the internal structure to that. Overall I find EFI confusing and overly complicated.
Between the two extremes is:
- link the worlds via pointers
- move protocol implementations into the respective uclasses
- endow these uclasses with an implementation of the driver binding protocol
This is feasible with the available capacity and sufficient to cover your use case of plugable devices.
That sounds good.
I wonder what the EFI world will look like if all the handles and protocols are also DM devices. I don't expect that my patch will be upstreamed any time soon (or possibly forever not) So, instead of claiming the change would be meaningless, I'd welcome any suggestions, like what will happen if we merge/integrate EFI's A with U-Boot's B?
I'm willing to make best efforts to give such an idea a reality if possible. Then choices come after that.
Basic idea is
- efi_root is a DM device
- Any efi object, refered to by efi_handle_t in UEFI world, has a corresponding DM device.
EFI applications and drivers will create handles having no relation to the U-Boot world.
I suggest that we change that, i.e. that all devices in existence have a struct udevice. That way DM knows about everything and we don't have the strange parallel 'EFI' world. I don't see any need for it.
Simply, it would be nice that we can list all the applications and drivers loaded at one place, akin to linux's
- ls /proc/
- cat /proc/modules
(From the viewpoint of API, we can do that just by calling locate_handle(BY_PROTOCOL, EFI_LOADED_IMAGE, ...) though.)
- define efi_handle_t as "struct udevice *"
An EFI handle does not necessarily relate to any U-Boot device. Why should a handle which has not backing device carry the extra fields of struct udevice?
Because this is the U-Boot driver model. We should not have an EFI parallel to DM and certainly not just to save a few dozen bytes of data space. If you were trying to save data space, you would not use EFI :-)
Ah, thank you.
From a viewpoint of implementation, the situation where some handles
are DM devices and some are not could make the efi code, particularly boottime.c, quite ugly and complicated.
- for efi_disk,
- add "struct efi_disk_obj" to blk_desc
struct efi_disk_obj * is currently the handle of the block device. So you only some fields may be moved to blk_desc. But I guess I will find that in one of the patches.
This is definitely a future work item. In this case, however, blk_desc should also be able to represent a partition.
- for the objects below, there is only one instance for each and so they are currently global data: efi_gop_obj, efi_net_obj,
efi_net_obj * is the handle of the network device. In future we should support multiple network devices.
It will be a natural extension.
simple_text_output_mode
- for loaded_image,
- link efi_loaded_image_obj to device's platdata
An EFI application can create an image out of "nothing". Just create a handle with InstallProtocolInterface() and then call LoadImage() with a pointer to some place in memory.
Many images loaded from the same device may be present at the same time, e.g. iPXE, GRUB, and Linux.
I don't get your point here, but please notice that a "loaded image" is more or less portion of main memory with loaded code. iPXE, GRUB and Linux are the same in this respect.
Why do you want to treat such a memory area as a separate device?
- Any efi protocol has a corresponding DM device.
Protocol implementations are not only provided by U-Boot but also by EFI applications and driver binaries. And of cause the EFI binaries will implement a lot of protocols completely unknown to U-Boot. So what should be the meaning of the above sentence in this context?
Can we instead add a uclass for each EFI protocol? Then U-Boot does know about them.
Yeah, I thought about defining an uclass for each EFI protocol. Given that a protocol defines a protocol-specific set of function interfaces in most cases, it will be natural to define a separate uclass.
An EFI binary can implement any EFI protocol that only exists for this special application. E.g. somebody could write an EFI driver implementing NVME over TCP.
Requiring that U-Boot has a uclass for every protocol would mean that at U-Boot compile time you would already have to define which EFI binaries a user is able to load. E.g. if a uclass for NVME over TCP does not exist a user would not be able to run above hypothetical binary.
Yes that's right, but I think that makes sense to have that support in U-Boot itself rather than out on a limb with EFI. It is natural consequence of fully using DM for EFI.
On the other hand, this will make it a bit complicated to determine whether a given handle is an efi object or efi protocol in DM tree.
In EFI terminology a protocol is not a handle but an abstract interface. The implementation of a protocol is called protocol interface.
A protocol interface may be installed on one or more handles. But it should not be enumerated by LocateHandles(SearchType = AllHandles)
I need to make time to read the EFI spec another 5 times :-) As I understand it the protocol corresponds to the operations in a U-Boot uclass.
Regarding a protocol "unknown to U-Boot," it is kinda headache as we can invent a totally *original* protocol which is unknown at compile time of U-Boot. That is one of reasons why all the protocols have the same type of uclass in my current implementation. (I don't think there is any way to define uclass dynamically.)
What is the benefit of protocol interface pointing to a uclass that doesn't implement the protocol?
I don't know about that, it doesn't seem useful.
Above you suggested that struct udevice * would be used as a handle. So on which handle is the protocol now installed in your model?
"efi_add_protocol" take a handle as a first argument, which is set to a parent of that protocol. If a handle is NULL here, a generated protocol handle will be temporarily attached to efi_root.
For a protocol like the device path protocol which is only a data structure with no related function modules I do not understand the benefit of creating a separate sub-device.
I think it is only a matter of convenience and to keep things regular.
Unifying device path hierarchy to DM tree is another challenge.
As an application may change the device path protocol of a handle at any time and we do not want to mess up the DM tree I would not see that this is possible.
What is the use case to changing the device path protocol?
- link "struct efi_handler" to device's uclass_platdata
struct efi_handler is an item in the list of protocols installed on a handle. For some of the protocols installed by an EFI application there will not be any corresponding uclass.
As above, perhaps we should fix that?
As I said above, I recognize that this is an issue.
- be a child of a efi object (hence DM device) in DM device hierarchy so that enumerating protocols belonging to efi object is done by traversing the tree.
Above you said a struct udevice * would be a handle. So here you imply that for each protocol interface you will create an extra handle. That does not fit the EFI model.
Please don't interpret the concept to such an extent. While, say, a GPIO has a DM device (and efi handle in this sense), is it also an efi object? No.
- Any efi object which has a backing DM device should be created when that DM device is detected (and probed).
Detected or probed? This is not the same.
So what is your suggestion here?
U-Boot follows the idea of late probing. So I guess we want an EFI handle to be created when the DM device is detected and probe it when the installed protocol is used for the first time.
That sound right to me.
- For efi_disk (or any object with EFI_BLOCK_IO_PROTOCOL),
- add UCLASS_PARTITION
- put partitions under a raw block device
- partitions as well as raw devices can be efi_disk
Agreed.
With those extensive changes, there still exists plenty of "wrapper" code. Do you have any idea to reduce it?
The concept presented does not cover:
- device, drivers, protocols created by EFI applications and driver binaries
I definitely want to see a good example so that I will investigate.
Can we create uclasses for each 'protocol'? Is there any reason why we cannot?
I think that I partly answered to you above.
- non-DM drivers and devices in U-Boot
This doesn't really matter as they will be gone soon. At the risk of repeating myself, EFI support should never have supported non-DM in the first place. It was not the right decision, in my view.
OK
It creates extra handles per installed protocol interface which should not exist in the EFI world.
So some rework of the concept is needed.
I suggest to start smaller:
- convert partitions to the DM model.
This is in later patches from what I can tell.
I would put it at the beginning as it is required for your use case of plugable devices.
Regards, Simon
Best regards
Heinrich
Yeah, to some extent. As I said above, blk_desc should be extended to support disk partitions on its own.
- provide a pointer serving as EFI handle in struct udevice
I actually feel that the approach here, while admittedly bold, seems to be a good step forward.
Regards, Simon
Best regards
Heinrich
Thank both of you for valuable comments.
-Takahiro Akashi
***** Example operation ****** (Two scsi disks, one with no partition, one with two partitions)
=> efi dev EFI: Initializing UCLASS_EFI_DRIVER Device Device Path ================ ==================== 000000007eef9470 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) => scsi rescan
Reset SCSI scanning bus for devices... Target spinup took 0 ms. Target spinup took 0 ms. SATA link 2 timeout. SATA link 3 timeout. SATA link 4 timeout. SATA link 5 timeout. AHCI 0001.0000 32 slots 6 ports 1.5 Gbps 0x3f impl SATA mode flags: 64bit ncq only Device 0: (0:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+ Type: Hard Disk Capacity: 16.0 MB = 0.0 GB (32768 x 512) Device 0: (1:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+ Type: Hard Disk Capacity: 256.0 MB = 0.2 GB (524288 x 512) => efi dev Device Device Path ================ ==================== 000000007eef9470 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) 000000007ef01c90 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0) 000000007ef04910 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0) 000000007ef04ee0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(335544330,MBR,0x086246ba,0x17ff4f1a0,0x7eee3770) 000000007ef055a0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(335544330,MBR,0x086246ba,0x17ff4f1a0,0x7eee3770) => dm tree Class index Probed Driver Name
root 0 [ + ] root_driver root_driver simple_bus 0 [ ] generic_simple_bus |-- platform@c000000 virtio 0 [ + ] virtio-mmio |-- virtio_mmio@a000000
[snip]
pci 0 [ + ] pci_generic_ecam |-- pcie@10000000 pci_generi 0 [ ] pci_generic_drv | |-- pci_0:0.0 virtio 32 [ ] virtio-pci.l | |-- virtio-pci.l#0 ahci 0 [ + ] ahci_pci | `-- ahci_pci scsi 0 [ + ] ahci_scsi | `-- ahci_scsi blk 0 [ + ] scsi_blk | |-- ahci_scsi.id0lun0 efi_protoc 8 [ + ] efi_disk | | |-- BLOCK_IO efi_protoc 9 [ + ] efi_device_path | | `-- Scsi(0,0) blk 1 [ + ] scsi_blk | `-- ahci_scsi.id1lun0 efi_protoc 10 [ + ] efi_disk | |-- BLOCK_IO efi_protoc 11 [ + ] efi_device_path | |-- Scsi(1,0) partition 0 [ + ] blk_partition | |-- ahci_scsi.id1lun0:1 efi_protoc 12 [ + ] efi_disk | | |-- BLOCK_IO efi_protoc 13 [ + ] efi_device_path | | |-- HD(335544330,MBR,0x086246ba,0x17ff4f1a0,0x7eee3770) efi_protoc 14 [ + ] efi_simple_file_syst | | `-- SIMPLE_FILE_SYSTEM partition 1 [ + ] blk_partition | `-- ahci_scsi.id1lun0:2 efi_protoc 15 [ + ] efi_disk | |-- BLOCK_IO efi_protoc 16 [ + ] efi_device_path | |-- HD(335544330,MBR,0x086246ba,0x17ff4f1a0,0x7eee3770) efi_protoc 17 [ + ] efi_simple_file_syst | `-- SIMPLE_FILE_SYSTEM rtc 0 [ ] rtc-pl031 |-- pl031@9010000 serial 0 [ ] serial_pl01x |-- pl011@9050000 serial 1 [ + ] serial_pl01x |-- pl011@9000000 efi_protoc 0 [ + ] efi_simple_text_outp | |-- SIMPLE_TEXT_OUTPUT efi_protoc 1 [ + ] efi_simple_text_inpu | |-- SIMPLE_TEXT_INPUT efi_protoc 2 [ + ] efi_simple_text_inpu | `-- SIMPLE_TEXT_INPUT_EX mtd 0 [ + ] cfi_flash |-- flash@0 firmware 0 [ + ] psci |-- psci sysreset 0 [ ] psci-sysreset | `-- psci-sysreset efi 0 [ + ] efi_root `-- UEFI sub system efi_protoc 3 [ + ] efi_device_path |-- VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) efi_protoc 4 [ + ] efi_device_path_to_t |-- DEVICE_PATH_TO_TEXT efi_protoc 5 [ + ] efi_device_path_util |-- DEVICE_PATH_UTILITIES efi_protoc 6 [ + ] efi_unicode_collatio |-- en efi_driver 0 [ + ] EFI block driver `-- EFI block driver efi_protoc 7 [ + ] efi_driver_binding `-- DRIVER_BINDING
Thanks, -Takahiro Akashi
AKASHI Takahiro (15): efi_loader: efi objects and protocols as DM devices efi_loader: boottime: convert efi_loaded_image_obj to DM efi_loader: image_loader: aligned with DM efi_driver: rename UCLASS_EFI to UCLASS_EFI_DRIVER efi_loader: convert efi_root_node to DM efi_loader: device path: convert efi_device_path to DM efi_loader: unicode_collation: converted to DM efi_loader: console: convert efi console input/output to DM efi_loader: net: convert efi_net_obj to DM efi_loader: gop: convert efi_gop_obj to DM dm: blk: add UCLASS_PARTITION efi_loader: disk: convert efi_disk_obj to DM drivers: align block device drivers with DM-efi integration efi_driver: converted to DM cmd: efidebug: aligned with DM-efi integration
cmd/bootefi.c | 61 +-- cmd/efidebug.c | 5 +- common/usb_storage.c | 27 +- drivers/block/blk-uclass.c | 61 +++ drivers/scsi/scsi.c | 22 + drivers/serial/serial-uclass.c | 6 + drivers/video/video-uclass.c | 9 + include/blk.h | 24 + include/dm/device.h | 3 + include/dm/uclass-id.h | 6 +- include/efi.h | 4 +- include/efi_loader.h | 50 +- lib/efi_driver/efi_block_device.c | 36 +- lib/efi_driver/efi_uclass.c | 37 +- lib/efi_loader/efi_boottime.c | 605 ++++++++++++++------- lib/efi_loader/efi_console.c | 64 ++- lib/efi_loader/efi_device_path.c | 136 +++-- lib/efi_loader/efi_device_path_to_text.c | 55 ++ lib/efi_loader/efi_device_path_utilities.c | 14 + lib/efi_loader/efi_disk.c | 216 +++++--- lib/efi_loader/efi_file.c | 14 + lib/efi_loader/efi_gop.c | 28 +- lib/efi_loader/efi_image_loader.c | 61 ++- lib/efi_loader/efi_net.c | 50 +- lib/efi_loader/efi_root_node.c | 14 +- lib/efi_loader/efi_setup.c | 60 +- lib/efi_loader/efi_unicode_collation.c | 19 + net/eth-uclass.c | 5 + 28 files changed, 1226 insertions(+), 466 deletions(-)