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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Mon, Oct 04, 2021 at 04:47:53PM +0200, Heinrich Schuchardt wrote:
> 
> 
> On 10/4/21 05:44, AKASHI Takahiro wrote:
> > # Resending the RFC as some of patches were deplicately submitted.
> > # See also
> >    https://git.linaro.org/people/takahiro.akashi/u-boot.git efi/dm_disk
> > 
> > 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.
> > 
> > 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.)
> > 
> > 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.
> > 5. There is no way supported to remove efi_disk's even after
> >     disconnect_controller() is called.
> > 
> > 
> > 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
> > 
> > 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.
> > 
> > 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).
> > 
> > 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")
> 
> An EFI application can create handles with any combination of protocols,
> e.g. a handle with both the block IO protocol and the simple network
> protocol. This means that a udevice cannot be assigned to a handle
> created by an EFI application.

Can you please elaborate more to clarify your point/suggestion here?

> When the EFI application calls ConnectController() for the handle,
> U-Boot can create child controllers. If U-Boot creates a udevice for
> such a child controller, it has to store the udevice pointer.
> lib/efi_driver/efi_block_device.c uses a private data section but you it
> could be preferable to use a field in struct efi_obj.

Before submitting this RFC, I also thought of a possibility of
re-implementing lib/efi_loader/efi_disk.c by defining a "controller"
for each U-Boot's block device (udevice) which is essentially a source
of providing BLOCK_IO_PROTOCOL as "efi_disk" devices and then implementing
"bind" interface of DRIVER_BINDING_PROTOCOL to create a mapping between
udevice(UCLASS_BLK) and efi_disk.
(Then I hoped we could reuse efi_driver framework for the case (1) below.)
Is this similar to what you think of here?

As I mentioned, there are two paths in creating efi_disks:
1) U-Boot's block device => efi_disk
   (efi_disk_add_dev() in lib/efi_loader/efi_disk.c is responsible for this.)
2) EFI app/driver -> efi_disk => U-Boot's block device
   (efi_bl_bind() in lib/efi_driver/efi_block_device.c)

Those two methods try to establish the relationship in opposite directions.
This is somewhat a cause of confusion/misunderstanding.


> > 
> > 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.
> > 
> > 4. I have no answer to issue(4) and (5) yet.
> 
> 4) A udevice shall only exist for the child controller handle created by
> U-Boot and not for the controller handle created by an EFI application.

I don't know what is a "child" controller, and will think of it.

> 5) The stop() method of the driver binding protocol has to take care of
> destroying the child controllers and the associated udevices.

That is a missing piece of code.

-Takahiro Akashi


> Best regards
> 
> Heinrich