mbox series

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

Message ID 20211004034430.41355-1-takahiro.akashi@linaro.org
Headers show
Series efi_loader: more tightly integrate UEFI disks to device model | expand

Message

AKASHI Takahiro Oct. 4, 2021, 3:44 a.m. UTC
# 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")

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.


Patchs:
=======
For easy understandings, patches may be categorized into seperate groups
of changes.

Patch#1-#9: DM: create udevices for partitions
Patch#10,#11: UEFI: use udevice interfaces
Patch#12-#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


[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):
  part: call part_init() in blk_get_device_by_str() only for MMC
  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

 common/usb_storage.c                         |   6 +
 disk/part.c                                  |   3 +-
 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 |  26 +-
 21 files changed, 566 insertions(+), 175 deletions(-)

-- 
2.33.0

Comments

Heinrich Schuchardt Oct. 4, 2021, 2:47 p.m. UTC | #1
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.

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.

>

> 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
Ilias Apalodimas Oct. 4, 2021, 6:07 p.m. UTC | #2
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
Simon Glass Oct. 4, 2021, 11:45 p.m. UTC | #3
Hi Takahiro,

On Sun, 3 Oct 2021 at 21:44, AKASHI Takahiro <takahiro.akashi@linaro.org> 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.


[..]

I expect to be able to dig into this at the end of the week.

Regards,
Simon


Regards,
Simon
AKASHI Takahiro Oct. 5, 2021, 2:14 a.m. UTC | #4
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
AKASHI Takahiro Oct. 5, 2021, 2:27 a.m. UTC | #5
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