Re: [PATCH v2 00/20] efi_loader: more tightly integrate UEFI disks to driver model

10 Feb 2022

      On 2/10/22 09:11, AKASHI Takahiro wrote:
...
Background:
The purpose of this patch is to reignite the discussion about how UEFI
subystem would best be integrated into U-Boot driver model.
In the past, I proposed 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.
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 enumerated 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:

While an efi_disk represents a device equally for either a whole disk
 or a partition in UEFI world, the driver model treats only a whole
 disk as a real block device or udevice(UCLASS_BLK).

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 driver model.
 # This issue, though, exists for all the implementation of U-Boot
 # file systems as well.

For efi_disk(a),
3. A block device can be enumerated 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
    operations.
5. There is no way supported to remove efi_disk's even after
    disconnect_controller() is called.
My approach:
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 partitions 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 enumerate partitions as well in there.

add new block access interfaces, which takes a *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 between a udevice and an efi_disk
    by adding
    - a UEFI handle pointer as a tag for a udevice
    - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj")
3-2. synchronize the lifetime of efi_disk objects in UEFI world with
    the driver model using
    - event notification associated with device's probe/remove.

I have no solution to issue(4) and (5) yet.

<<<Example DM tree on qemu-arm64>>>
=> dm tree
  Class      Driver               Name

root       root_driver          root_driver
  ...
  pci        pci_generic_ecam     |-- pcie@10000000
  pci_generi pci_generic_drv      |   |-- pci_0:0.0
  virtio     virtio-pci.l         |   |-- virtio-pci.l#0
  ethernet   virtio-net           |   |   `-- virtio-net#32
  ahci       ahci_pci             |   |-- ahci_pci
  scsi       ahci_scsi            |   |   `-- ahci_scsi
  blk        scsi_blk             |   |       |-- ahci_scsi.id0lun0
  partition  blk_partition        |   |       |   |-- ahci_scsi.id0lun0:1
  partition  blk_partition        |   |       |   `-- ahci_scsi.id0lun0:2
  blk        scsi_blk             |   |       `-- ahci_scsi.id1lun0
  partition  blk_partition        |   |           |-- ahci_scsi.id1lun0:1
  partition  blk_partition        |   |           `-- ahci_scsi.id1lun0:2
  usb        xhci_pci             |   `-- xhci_pci
  usb_hub    usb_hub              |       `-- usb_hub
  usb_dev_ge usb_dev_generic_drv  |           |-- generic_bus_0_dev_2
  usb_mass_s usb_mass_storage     |           `-- usb_mass_storage
  blk        usb_storage_blk      |               `-- usb_mass_storage.lun0
  partition  blk_partition        |                   |-- usb_mass_storage.lun0:1
  partition  blk_partition        |                   `-- usb_mass_storage.lun0:2
  ...
=> efi devices
Device           Device Path
================ ====================
000000013eeea8d0 /VenHw()
000000013eeed810 /VenHw()/MAC(525252525252,1)
000000013eefc460 /VenHw()/Scsi(0,0)
000000013eefc5a0 /VenHw()/Scsi(0,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a)
000000013eefe320 /VenHw()/Scsi(0,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800)
000000013eeff210 /VenHw()/Scsi(1,0)
000000013eeff390 /VenHw()/Scsi(1,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a)
000000013eeff7d0 /VenHw()/Scsi(1,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800)
000000013ef04c20 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)
000000013ef04da0 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(1,0x01,0,0x0,0x99800)
000000013ef04f70 /VenHw()/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(2,0x01,0,0x99800,0x1800)
Patchs:
For easy understandings, patches may be categorized into separate groups
of changes.
Patch#1-#7: DM: add device_probe() for later use of events
Patch#8-#11: DM: add new features (tag and event notification)
Patch#12-#16: UEFI: dynamically create/remove efi_disk's for a raw disk
   and its partitions
   For removal case, we may need more consideration since removing handles
   unconditionally may end up breaking integrity of handles
   (as some may still be held and referenced to by a UEFI app).
Patch#17-#18: UEFI: use udevice read/write interfaces
Patch#19-#20: UEFI: fix-up efi_driver, aligning with changes in DM 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
This series does not pass Gitlab CI:
See
https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391030
https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/391031
I will set the whole series to "changes requested"
Please, run 'make tests' before resubmitting.
Best regards
Heinrich
=================================== FAILURES
===================================
________________________________ test_gpt_write
________________________________
test/py/tests/test_gpt.py:169: in test_gpt_write
     assert 'Writing GPT: success!' in output
E   AssertionError: assert 'Writing GPT: success!' in 'Writing GPT: Not
a block device: rng\r\r\nsuccess!'
----------------------------- Captured stdout call
-----------------------------
=> host bind 0 /tmp/sandbox/test_gpt_disk_image.bin
=> => gpt write host 0 "name=all,size=0"
Writing GPT: Not a block device: rng
success!
=>
___________________ test_ut[ut_dm_dm_test_video_comp_bmp32]
____________________
test/py/tests/test_ut.py:43: in test_ut
     assert output.endswith('Failures: 0')
E   AssertionError: assert False
E    +  where False = <built-in method endswith of str object at
0x7fd72d2fc800>('Failures: 0')
E    +    where <built-in method endswith of str object at
0x7fd72d2fc800> = 'Test: dm_test_video_comp_bmp32: video.c\r\r\nSDL
renderer does not exist\r\r\ntest/dm/video.c:88,
compress_frame_buff..._test_video_comp_bmp32(): 2024 ==
compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1
(1)\r\r\nFailures: 2'.endswith
----------------------------- Captured stdout call
-----------------------------
=> ut dm dm_test_video_comp_bmp32
Test: dm_test_video_comp_bmp32: video.c
SDL renderer does not exist
test/dm/video.c:88, compress_frame_buffer(): !memcmp(uc_priv->fb,
uc_priv->copy_fb, uc_priv->fb_size): Copy framebuffer does not match fb
test/dm/video.c:484, dm_test_video_comp_bmp32(): 2024 ==
compress_frame_buffer(uts, dev): Expected 0x7e8 (2024), got 0x1 (1)
Failures: 2
=>
_______________________________ test_avb_read_rb
_______________________________
test/py/tests/test_android/test_avb.py:83: in test_avb_read_rb
     assert response == 'Rollback index: 0'
E   AssertionError: assert 'Not a block ...back index: 0' == 'Rollback
index: 0'
E     - Not a block device: sandbox_tee
E     -
E       Rollback index: 0
----------------------------- Captured stdout call
-----------------------------
=> avb init 1
=> => avb read_rb 1
Not a block device: sandbox_tee
Rollback index: 0
=>
_____________________________ test_avb_is_unlocked
_____________________________
test/py/tests/test_android/test_avb.py:95: in test_avb_is_unlocked
     assert response == 'Unlocked = 1'
E   AssertionError: assert 'Not a block ...nUnlocked = 1' == 'Unlocked = 1'
E     - Not a block device: sandbox_tee
E     -
E       Unlocked = 1
---------------------------- Captured stdout setup
-----------------------------
/u-boot
U-Boot 2022.04-rc1-00209-g173fff8119 (Feb 10 2022 - 14:59:41 +0000)
Model: sandbox
DRAM:  128 MiB
Core:  248 devices, 90 uclasses, devicetree: board
WDT:   Not starting gpio-wdt
WDT:   Not starting wdt@0
MMC:   mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
Loading Environment from nowhere... OK
In:    cros-ec-keyb
Out:   vidconsole
Err:   vidconsole
Model: sandbox
SCSI:
Net:   eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6:
eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1
7[r[999;999H[6n8Not a block device: pinmux_i2c0_pins
Not a block device: i2c@0
Not a block device: rtc@61
Not a block device: bootcount@0
Not a block device: emul
Not a block device: emull
Hit any key to stop autoboot:  2  0
=>
----------------------------- Captured stdout call
-----------------------------
=> avb init 1
=> => avb is_unlocked
Not a block device: sandbox_tee
Unlocked = 1
=>
__________________________ test_avb_persistent_values
__________________________
test/py/tests/test_android/test_avb.py:134: in test_avb_persistent_values
     assert response == 'Wrote 12 bytes'
E   AssertionError: assert 'Not a block ...rote 12 bytes' == 'Wrote 12
bytes'
E     - Not a block device: sandbox_tee
E     -
E       Wrote 12 bytes
---------------------------- Captured stdout setup
-----------------------------
/u-boot
U-Boot 2022.04-rc1-00209-g173fff8119 (Feb 10 2022 - 14:59:41 +0000)
Model: sandbox
DRAM:  128 MiB
Core:  248 devices, 90 uclasses, devicetree: board
WDT:   Not starting gpio-wdt
WDT:   Not starting wdt@0
MMC:   mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
Loading Environment from nowhere... OK
In:    cros-ec-keyb
Out:   vidconsole
Err:   vidconsole
Model: sandbox
SCSI:
Net:   eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6:
eth@10004000, eth4: dsa-test-eth, eth2: lan0, eth7: lan1
7[r[999;999H[6n8Not a block device: pinmux_i2c0_pins
Not a block device: i2c@0
Not a block device: rtc@61
Not a block device: bootcount@0
Not a block device: emul
Not a block device: emull
Hit any key to stop autoboot:  2  0
=>
----------------------------- Captured stdout call
-----------------------------
=> avb init 1
=> => avb write_pvalue test value_value
Not a block device: sandbox_tee
Wrote 12 bytes
=>
...
Change history:
v2 (Feb 10, 2022)

add/revise an error message if device_probe() fails (patch#3,#5)
fix a build error in sandbox_spl_defconfig (patch#8)
fix warnings in 'make htmldocs' (patch#8,#9,#18)
new commit: split efi_init_obj_list() (patch#14)

v1 (Feb 2, 2022)

rebased on 2022.04-rc1
drop patches that have already been merged
modify a tag-range check with "tag >= DM_TAG_COUNT" (patch#9)
move dmtag_list to GD (global data) (patch#9)
add function descriptions and a document about DM tag feature (patch#9,10)
add tests for DM tag support (patch#11)
change 'depends on EVENT' to 'select EVENT' for EFI_LOADER (patch#14)
migrate IF_TYPE_EFI to IF_TYPE_EFI_LOADER (patch#18)

RFCv2 (Dec 10, 2021)

rebased on 2022-rc3
re-order and merge some related commits into ones
call device_probe() in MMC (not bind, but) probe hook (patch#5)
fix a wrong name of variable (patch#7)
add patch#9
invoke device_probe() for virtio devices (patch#10)
add DM event notification (from Simon) (patch#11)
add DM tag support (patch#12)
move UCLASS_PARTITION driver under disk/ (patch#13)
create partition's dp using its parent's. This change is necessary
 in particular for 'efi_blk' efi_disk (patch#13)
modify the code so that we will use new features like tags and
 event notification (patch#13,15,16,20)
rename new functions from blk_read/write() to dev_read/write()
 (patch#17,18)
isolate changes in efi_driver from the rest (in efi_loader) (patch#19)
drop the previous patch#22 ("efi_selftest: block device: adjust dp
 for a test") due to the fix in patch#13

RFC (Nov 16, 2021)

initial RFC

AKASHI Takahiro (19):
   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
   virtio: call device_probe() in scanning
   dm: add tag support
   dm: tag: add some document
   test: dm: add tests for tag support
   dm: disk: add UCLASS_PARTITION
   dm: blk: add a device-probe hook for scanning disk partitions
   efi_loader: split efi_init_obj_list() into two stages
   efi_loader: disk: a helper function to create efi_disk objects from
     udevice
   efi_loader: disk: a helper function to delete efi_disk objects
   dm: disk: add read/write interfaces with udevice
   efi_loader: disk: use udevice instead of blk_desc
   efi_loader: disk: not create BLK device for BLK(IF_TYPE_EFI_LOADER)
     devices
   efi_driver: align with efi_disk-dm integration
Simon Glass (1):
   dm: add event notification
cmd/virtio.c                        |  21 +-
  common/Kconfig                      |  11 +
  common/Makefile                     |   2 +
  common/board_f.c                    |   2 +
  common/board_r.c                    |   2 +-
  common/event.c                      | 103 +++++++++
  common/log.c                        |   1 +
  common/main.c                       |   7 +-
  common/usb_storage.c                |   4 +
  disk/Makefile                       |   3 +
  disk/disk-uclass.c                  | 247 +++++++++++++++++++++
  doc/develop/driver-model/design.rst |  20 ++
  drivers/ata/dwc_ahsata.c            |   5 +
  drivers/ata/fsl_sata.c              |  11 +
  drivers/ata/sata_mv.c               |   5 +
  drivers/ata/sata_sil.c              |  12 +
  drivers/block/blk-uclass.c          |   4 +
  drivers/block/ide.c                 |   4 +
  drivers/core/Makefile               |   2 +-
  drivers/core/device-remove.c        |   9 +
  drivers/core/device.c               |   9 +
  drivers/core/root.c                 |   2 +
  drivers/core/tag.c                  | 139 ++++++++++++
  drivers/mmc/mmc-uclass.c            |  12 +
  drivers/nvme/nvme.c                 |   4 +
  drivers/scsi/scsi.c                 |   5 +
  include/asm-generic/global_data.h   |  10 +
  include/dm/device-internal.h        |  10 +
  include/dm/tag.h                    | 110 +++++++++
  include/dm/uclass-id.h              |   1 +
  include/efi_loader.h                |   6 +-
  include/event.h                     | 105 +++++++++
  include/event_internal.h            |  34 +++
  include/log.h                       |   2 +
  include/part.h                      |  18 ++
  lib/efi_driver/efi_block_device.c   |  34 +--
  lib/efi_loader/Kconfig              |   2 +
  lib/efi_loader/efi_disk.c           | 331 ++++++++++++++++++++--------
  lib/efi_loader/efi_setup.c          |  62 +++++-
  test/common/Makefile                |   1 +
  test/common/event.c                 |  87 ++++++++
  test/dm/Makefile                    |   1 +
  test/dm/tag.c                       |  80 +++++++
  test/test-main.c                    |   7 +
  44 files changed, 1416 insertions(+), 131 deletions(-)
  create mode 100644 common/event.c
  create mode 100644 disk/disk-uclass.c
  create mode 100644 drivers/core/tag.c
  create mode 100644 include/dm/tag.h
  create mode 100644 include/event.h
  create mode 100644 include/event_internal.h
  create mode 100644 test/common/event.c
  create mode 100644 test/dm/tag.c

Re: [PATCH v2 00/20] efi_loader: more tightly integrate UEFI disks to driver model

Heinrich Schuchardt

Background:

Disks in UEFI world:

Issues:

My approach:

Patchs:

Change history: