Hi Heinrich, On Fri, 8 Feb 2019 at 03:36, Heinrich Schuchardt <[email protected]> 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. > > > 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. > > > - 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 :-) > > > - 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. > > > - 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. > > > 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. > > > > > * 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. > > Above you suggested that struct udevice * would be used as a handle. > So on which handle is the protocol now installed in your model? > > 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. > > > - 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? > > > - 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. > > > * 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. > > > * 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 Can we create uclasses for each 'protocol'? Is there any reason why we cannot? > - 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. > > 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. > - 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 > > > > > ***** 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 [email protected] https://lists.denx.de/listinfo/u-boot
