Hi Heinrich, On Tue, 12 Feb 2019 at 17:53, Heinrich Schuchardt <xypron.g...@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.g...@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(-) > >>>> > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
