Heinrich,
On Thu, Feb 10, 2022 at 04:20:11PM +0100, Heinrich Schuchardt wrote:
> 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:
> > =======
> > 1. 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).
> >
> > 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 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.
> >
> > 2. 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.
> >
> > 4. 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 have noticed those errors but I didn't think that they were related
to my patch set initially as I didn't touch any code in gpt driver,
android/avb nor video driver.
> 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!'
The reason of assertion failure here is that some log message was
inserted in a output message although the test itself was finished
successfully:
"Writing GPT: success!" <== a correct output message
^
"Not a block device: rng"
Adding efi_disk_probe() as a callback to EVT_DM_POST_PROBE created
this *log_info* message in dm_rng_read() <- get_rand_uuid() <-
gen_rand_uuid_str() in "gpt write" command.
We can fix this type of failure by the hack:
===8<===
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -612,8 +612,6 @@ static int efi_disk_probe(void *ctx, struct event *event)
/* TODO: We won't support partitions in a partition */
if (id != UCLASS_BLK) {
- if (id != UCLASS_PARTITION)
- log_info("Not a block device: %s\n", dev->name);
return 0;
}
===>8===
I don't think, however, that it is a good thing that test results
depend on console outputs, especially *log* messages.
Furthermore, I don't know why we see *info*-level messages
even under CONFIG_LOGLEVEL=4 (warning).
> ----------------------------- 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
I don't know yet why this happened.
> =>
> _______________________________ 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
The same error as mentioned above.
-Takahiro Akashi
> 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
> >
>
