[RFC 00/22] efi_loader: more tightly integrate UEFI disks to device model
Simon Glass
sjg at chromium.org
Sun Oct 10 16:14:00 CEST 2021
Hi Takahiro,
On Thu, 30 Sept 2021 at 23:02, AKASHI Takahiro
<takahiro.akashi at linaro.org> wrote:
>
> The purpose of this RPC is to reignite the discussion about how UEFI
> subystem would best be integrated into U-Boot device model.
> In the past, I poposed a couple of patch series, the latest one[1],
> while Heinrich revealed his idea[2], and the approach taken here is
> something between them, with a focus on block device handlings.
>
> # The code is a PoC and not well tested yet.
>
> Disks in UEFI world:
> ====================
> In general in UEFI world, accessing to any device is performed through
> a 'protocol' interface which are installed to (or associated with) the device's
> UEFI handle (or an opaque pointer to UEFI object data). Protocols are
> implemented by either the UEFI system itself or UEFI drivers.
>
> For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk
> hereafter). Currently, every efi_disk may have one of two origins:
> a.U-Boot's block devices or related partitions
> (lib/efi_loader/efi_disk.c)
> b.UEFI objects which are implemented as a block device by UEFI drivers.
> (lib/efi_driver/efi_block_device.c)
>
> All the efi_diskss as (a) will be enumelated and created only once at UEFI
> subsystem initialization (efi_disk_register()), which is triggered by
> first executing one of UEFI-related U-Boot commands, like "bootefi",
> "setenv -e" or "efidebug".
> EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops)
> in the corresponding udevice(UCLASS_BLK).
>
> On the other hand, efi_disk as (b) will be created each time UEFI boot
> services' connect_controller() is executed in UEFI app which, as a (device)
> controller, gives the method to access the device's data,
> ie. EFI_BLOCK_IO_PROTOCOL.
>
> >>> more details >>>
> Internally, connect_controller() search for UEFI driver that can support
> this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case,
> then calls the driver's 'bind' interface, which eventually installs
> the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object.
> 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for
> * creating additional partitions efi_disk's, and
> * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it.
> <<< <<<
>
> Issues:
> =======
> 1. While an efi_disk represents a device equally for either a whole disk
> or a partition in UEFI world, the device model treats only a whole
> disk as a real block device or udevice(UCLASS_BLK).
>
> 2. efi_disk holds and makes use of "blk_desc" data even though blk_desc
> in plat_data is supposed to be private and not to be accessed outside
> the device model.
> # This issue, though, exists for all the implmenetation of U-Boot
> # file systems as well.
Yes, this was a migration convenience and we should be able to unpick it now.
However we still have SPL_BLK so need to consider whether we can drop that.
>
> For efi_disk(a),
> 3. A block device can be enumelated dynamically by 'scanning' a device bus
> in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly.
> For examples,
> => scsi rescan; efidebug devices
> => usb start; efidebug devices ... (A)
> (A) doesn't show any usb devices detected.
>
> => scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ...
> => scsi rescan ... (B)
> => bootefi bootmgr ... (C)
> (C) may de-reference a bogus blk_desc pointer which has been freed by (B).
> (Please note that "scsi rescan" removes all udevices/blk_desc and then
> re-create them even if nothing is changed on a bus.)
This is something your series will help with.
>
> For efi_disk(b),
> 4. A controller (handle), combined with efi_block driver, has no
> corresponding udevice as a parent of efi_disks in DM tree, unlike, say,
> a scsi controller, even though it provides methods for block io perations.
Can we just create a udevice?
> 5. There is no way supported to remove efi_disk's even after
> disconnect_controller() is called.
>
Do we want to remove disks? I don't really understand the context, but
if EFI wants to forget about a disk, that should not affect the rest
of U-Boot.
>
> My approach in this RFC:
> ========================
> Due to functional differences in semantics, it would be difficult
> to identify "udevice" structure as a handle in UEFI world. Instead, we will
> have to somehow maintain a relationship between a udevice and a handle.
>
> 1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions
> Currently, the uclass for paritions is not a UCLASS_BLK.
> It can be possible to define partitions as UCLASS_BLK
> (with IF_TYPE_PARTION?), but
> I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK)
> is tightly coupled with 'struct blk_desc' data which is still used
> as a "structure to a whole disk" in a lot of interfaces.
> (I hope that you understand what it means.)
>
> In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent:
> For instance,
> UCLASS_SCSI --- UCLASS_BLK --- UCLASS_PARTITION
> (IF_TYPE_SCSI) |
> +- struct blk_desc +- struct disk_part
> +- scsi_blk_ops +- blk_part_ops
Seems right to me. But I think a partition should also have a BLK
child, so that only block devices can be read/written. In fact,
perhaps a partition should be a child of the media device (e.g. MMC),
not of the BLK device? That might make more sense.
>
> 1-2. create partition udevices in the context of device_probe()
> part_init() is already called in blk_post_probe(). See the commit
> d0851c893706 ("blk: Call part_init() in the post_probe() method").
> Why not enumelate partitions as well in there.
Sounds reasonable.
>
> 2. add new block access interfaces, which takes "udevice" as a target device,
> in U-Boot and use those functions to implement efi_disk operations
> (i.e. EFI_BLOCK_IO_PROTOCOL).
Nice.
>
> 3-1. maintain a bi-directional link by adding
> - a UEFI handle pointer in "struct udevice"
> - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")
We should generalise the API here, but otherwise seems right. See my
comment on the patch.
>
> 3-2. use device model's post_probe/pre_remove hook to synchronize the lifetime
> of efi_disk objects in UEFI world with the device model.
We should add an event mechanism to generalise this a bit. See my
comment on the patch.
>
> 4. I have no answer to issue(4) and (5) yet.
See above.
>
>
> Patchs:
> =======
> For easy understandings, patches may be categorized into seperate groups
> of changes.
>
> Patch#1-#9: DM: create udevices for partitions
> Patch#11,#12: UEFI: use udevice interfaces
> Patch#13-#16: UEFI: sync up with the device model for adding
> Patch#17-#19: UEFI: sync up with the device model for removing
> For removal case, we may need more consideration since removing handles
> unconditionally may end up breaking integrity of handles
> (some may still be held and referenced to by a UEFI app).
> Patch#20-#22: UEFI: align efi_driver with changes by the integration
>
This looks great to me and is the first real step I have seen to
actually solving the problem of all this duplication.
I think we need a few more DM APIs as I suggest in a few of the
patches. I am happy to help with these if you like.
I wonder if it is possible to get some version of at least part of
this code applied in this merge window? It helps to set things on a
much better direction.
I also worry that continuing to develop the EFI impl while it is such
a sorry state is adding to the difficulty of fixing it up.
Regards,
Simon
>
> [1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html
> [2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html
>
> AKASHI Takahiro (22):
> scsi: call device_probe() after scanning
> usb: storage: call device_probe() after scanning
> mmc: call device_probe() after scanning
> nvme: call device_probe() after scanning
> sata: call device_probe() after scanning
> block: ide: call device_probe() after scanning
> dm: blk: add UCLASS_PARTITION
> dm: blk: add a device-probe hook for scanning disk partitions
> dm: blk: add read/write interfaces with udevice
> efi_loader: disk: use udevice instead of blk_desc
> dm: add a hidden link to efi object
> efi_loader: remove !CONFIG_BLK code from efi_disk
> efi_loader: disk: a helper function to create efi_disk objects from
> udevice
> dm: blk: call efi's device-probe hook
> efi_loader: cleanup after efi_disk-dm integration
> efi_loader: add efi_remove_handle()
> efi_loader: efi_disk: a helper function to delete efi_disk objects
> dm: blk: call efi's device-removal hook
> efi_driver: align with efi_disk-dm integration
> efi_driver: cleanup after efi_disk-dm integration
> efi_selftest: block device: adjust dp for a test disk
> (TEST) let dm-tree unchanged after block_io testing is done
>
> common/usb_storage.c | 6 +
> drivers/ata/dwc_ahsata.c | 10 +
> drivers/ata/fsl_sata.c | 11 +
> drivers/ata/sata_mv.c | 9 +
> drivers/ata/sata_sil.c | 12 +
> drivers/block/blk-uclass.c | 249 ++++++++++++++++
> drivers/block/ide.c | 6 +
> drivers/mmc/mmc-uclass.c | 7 +
> drivers/nvme/nvme.c | 6 +
> drivers/scsi/scsi.c | 10 +
> include/blk.h | 15 +
> include/dm/device.h | 4 +
> include/dm/uclass-id.h | 1 +
> include/efi_loader.h | 8 +-
> lib/efi_driver/efi_block_device.c | 30 +-
> lib/efi_loader/efi_boottime.c | 8 +
> lib/efi_loader/efi_device_path.c | 29 ++
> lib/efi_loader/efi_disk.c | 286 ++++++++++---------
> lib/efi_loader/efi_setup.c | 5 -
> lib/efi_selftest/efi_selftest_block_device.c | 28 +-
> 20 files changed, 566 insertions(+), 174 deletions(-)
>
> --
> 2.33.0
>
More information about the U-Boot
mailing list