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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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

> > 
> >     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.

> > 
> > 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