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 09:07:35PM +0300, Ilias Apalodimas wrote:
> On Mon, Oct 04, 2021 at 04:47:53PM +0200, Heinrich Schuchardt wrote:
> > 
> > 
> > > 
> 
> [...]
> 
> > > 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.)
> 
> I think it makes more sense the way it's currently defined.  I don;t see a
> point in hiding partitions within UCLASS_BLK

Yeah. But even with my UCLASS_PARTITION, it provides block-level io's
through blk_read/blk_write() APIs.
So someone may wonder why two different type of udevices have the same
interfaces :)

> > > 
> > >     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.
> > 
> > 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.
> > 
> 
> I agree with Heinrich here.  Basically U-Boot has to be in charge of that.
> Once ConnectController has been called U-Boot should create an 1:1 mapping
> between udevice <-> handle and shouldn't be allowed to change that.

Again, are you sure you're talking about the implementation of efi_disk for
U-Boot's block device (the path(1) in my previous reply to Heinrich)?

-Takahiro Akashi

> > > 
> > > 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.
> > 
> > 5) The stop() method of the driver binding protocol has to take care of
> > destroying the child controllers and the associated udevices.
> > 
> > Best regards
> > 
> > Heinrich
> 
> Thanks
> /Ilias