Re: [RFC 00/22] efi_loader: more tightly integrate UEFI disks to device model

[Simon Glass <sjg@chromium.org>]

Hi Takahiro,

On Thu, 30 Sept 2021 at 23:02, AKASHI Takahiro
<takahiro.akashi@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
>