[PATCH v5 07/12] efi_loader: disk: a helper function to create efi_disk objects from udevice
AKASHI Takahiro
takahiro.akashi at linaro.org
Wed Jun 1 02:25:18 CEST 2022
Hi Neil,
On Thu, May 19, 2022 at 02:04:53PM +0900, AKASHI Takahiro wrote:
> On Wed, May 18, 2022 at 11:23:32AM +0200, Neil Armstrong wrote:
> > On 19/04/2022 03:05, AKASHI Takahiro wrote:
> > > Add efi_disk_probe() function.
> > > This function creates an efi_disk object for a raw disk device (UCLASS_BLK)
> > > and additional objects for related partitions (UCLASS_PARTITION).
> > >
> > > So this function is expected to be called through driver model's "probe"
> > > interface every time one raw disk device is detected and activated.
> > > We assume that partition devices (UCLASS_PARTITION) have been created
> > > when this function is invoked.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > > ---
> > > include/efi_loader.h | 4 +-
> > > lib/efi_driver/efi_block_device.c | 34 ++---
> > > lib/efi_loader/Kconfig | 3 +
> > > lib/efi_loader/efi_disk.c | 201 +++++++++++++++++++-----------
> > > lib/efi_loader/efi_setup.c | 4 +-
> > > 5 files changed, 143 insertions(+), 103 deletions(-)
> > >
> > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > index 5bb8fb2acd04..ba79a9afb404 100644
> > > --- a/include/efi_loader.h
> > > +++ b/include/efi_loader.h
> > > @@ -525,8 +525,8 @@ void efi_carve_out_dt_rsv(void *fdt);
> > > void efi_try_purge_kaslr_seed(void *fdt);
> > > /* Called by bootefi to make console interface available */
> > > efi_status_t efi_console_register(void);
> > > -/* Called by bootefi to make all disk storage accessible as EFI objects */
> > > -efi_status_t efi_disk_register(void);
> > > +/* Called by efi_init_obj_list() to initialize efi_disks */
> > > +efi_status_t efi_disk_init(void);
> > > /* Called by efi_init_obj_list() to install EFI_RNG_PROTOCOL */
> > > efi_status_t efi_rng_register(void);
> > > /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
> > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c
> > > index 04cb3ef0d4e5..5baa6f87a375 100644
> > > --- a/lib/efi_driver/efi_block_device.c
> > > +++ b/lib/efi_driver/efi_block_device.c
> > > @@ -35,6 +35,7 @@
> > > #include <malloc.h>
> > > #include <dm/device-internal.h>
> > > #include <dm/root.h>
> > > +#include <dm/tag.h>
> > > /*
> > > * EFI attributes of the udevice handled by this driver.
> > > @@ -106,25 +107,6 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
> > > return blkcnt;
> > > }
> > > -/**
> > > - * Create partions for the block device.
> > > - *
> > > - * @handle: EFI handle of the block device
> > > - * @dev: udevice of the block device
> > > - * Return: number of partitions created
> > > - */
> > > -static int efi_bl_bind_partitions(efi_handle_t handle, struct udevice *dev)
> > > -{
> > > - struct blk_desc *desc;
> > > - const char *if_typename;
> > > -
> > > - desc = dev_get_uclass_plat(dev);
> > > - if_typename = blk_get_if_type_name(desc->if_type);
> > > -
> > > - return efi_disk_create_partitions(handle, desc, if_typename,
> > > - desc->devnum, dev->name);
> > > -}
> > > -
> > > /**
> > > * Create a block device for a handle
> > > *
> > > @@ -139,7 +121,6 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)
> > > char *name;
> > > struct efi_object *obj = efi_search_obj(handle);
> > > struct efi_block_io *io = interface;
> > > - int disks;
> > > struct efi_blk_plat *plat;
> > > EFI_PRINT("%s: handle %p, interface %p\n", __func__, handle, io);
> > > @@ -173,15 +154,20 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)
> > > plat->handle = handle;
> > > plat->io = interface;
> > > + /*
> > > + * FIXME: necessary because we won't do almost nothing in
> > > + * efi_disk_create() when called from device_probe().
> > > + */
> > > + ret = dev_tag_set_ptr(bdev, DM_TAG_EFI, handle);
> > > + if (ret)
> > > + /* FIXME: cleanup for bdev */
> > > + return ret;
> > > +
> > > ret = device_probe(bdev);
> > > if (ret)
> > > return ret;
> > > EFI_PRINT("%s: block device '%s' created\n", __func__, bdev->name);
> > > - /* Create handles for the partions of the block device */
> > > - disks = efi_bl_bind_partitions(handle, bdev);
> > > - EFI_PRINT("Found %d partitions\n", disks);
> > > -
> > > return 0;
> > > }
> > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > index bc518d7a413b..6b245f50a726 100644
> > > --- a/lib/efi_loader/Kconfig
> > > +++ b/lib/efi_loader/Kconfig
> > > @@ -14,6 +14,8 @@ config EFI_LOADER
> > > depends on DM_ETH || !NET
> > > depends on !EFI_APP
> > > default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8
> > > + select DM_EVENT
> > > + select EVENT_DYNAMIC
> > > select LIB_UUID
> > > imply PARTITION_UUIDS
> > > select HAVE_BLOCK_DEVICE
> > > @@ -40,6 +42,7 @@ config CMD_BOOTEFI_BOOTMGR
> > > config EFI_SETUP_EARLY
> > > bool
> > > + default y
> > > choice
> > > prompt "Store for non-volatile UEFI variables"
> > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > > index c905c12abc2f..d4a0edb458b8 100644
> > > --- a/lib/efi_loader/efi_disk.c
> > > +++ b/lib/efi_loader/efi_disk.c
> > > @@ -10,6 +10,9 @@
> > > #include <common.h>
> > > #include <blk.h>
> > > #include <dm.h>
> > > +#include <dm/device-internal.h>
> > > +#include <dm/tag.h>
> > > +#include <event.h>
> > > #include <efi_loader.h>
> > > #include <fs.h>
> > > #include <log.h>
> > > @@ -487,103 +490,153 @@ error:
> > > return ret;
> > > }
> > > -/**
> > > - * efi_disk_create_partitions() - create handles and protocols for partitions
> > > +/*
> > > + * Create a handle for a whole raw disk
> > > + *
> > > + * @dev uclass device (UCLASS_BLK)
> > > *
> > > - * Create handles and protocols for the partitions of a block device.
> > > + * Create an efi_disk object which is associated with @dev.
> > > + * The type of @dev must be UCLASS_BLK.
> > > *
> > > - * @parent: handle of the parent disk
> > > - * @desc: block device
> > > - * @if_typename: interface type
> > > - * @diskid: device number
> > > - * @pdevname: device name
> > > - * Return: number of partitions created
> > > + * @return 0 on success, -1 otherwise
> > > */
> > > -int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> > > - const char *if_typename, int diskid,
> > > - const char *pdevname)
> > > +static int efi_disk_create_raw(struct udevice *dev)
> > > {
> > > - int disks = 0;
> > > - char devname[32] = { 0 }; /* dp->str is u16[32] long */
> > > - int part;
> > > - struct efi_device_path *dp = NULL;
> > > + struct efi_disk_obj *disk;
> > > + struct blk_desc *desc;
> > > + const char *if_typename;
> > > + int diskid;
> > > efi_status_t ret;
> > > - struct efi_handler *handler;
> > > - /* Get the device path of the parent */
> > > - ret = efi_search_protocol(parent, &efi_guid_device_path, &handler);
> > > - if (ret == EFI_SUCCESS)
> > > - dp = handler->protocol_interface;
> > > -
> > > - /* Add devices for each partition */
> > > - for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> > > - struct disk_partition info;
> > > -
> > > - if (part_get_info(desc, part, &info))
> > > - continue;
> > > - snprintf(devname, sizeof(devname), "%s:%x", pdevname,
> > > - part);
> > > - ret = efi_disk_add_dev(parent, dp, if_typename, desc, diskid,
> > > - &info, part, NULL);
> > > - if (ret != EFI_SUCCESS) {
> > > - log_err("Adding partition %s failed\n", pdevname);
> > > - continue;
> > > - }
> > > - disks++;
> > > + desc = dev_get_uclass_plat(dev);
> > > + if_typename = blk_get_if_type_name(desc->if_type);
> > > + diskid = desc->devnum;
> > > +
> > > + ret = efi_disk_add_dev(NULL, NULL, if_typename, desc,
> > > + diskid, NULL, 0, &disk);
> > > + if (ret != EFI_SUCCESS) {
> > > + if (ret == EFI_NOT_READY)
> > > + log_notice("Disk %s not ready\n", dev->name);
> > > + else
> > > + log_err("Adding disk for %s failed\n", dev->name);
> > > +
> > > + return -1;
> > > + }
> > > + if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) {
> > > + efi_free_pool(disk->dp);
> > > + efi_delete_handle(&disk->header);
> > > +
> > > + return -1;
> > > }
> > > - return disks;
> > > + return 0;
> > > }
> > > -/**
> > > - * efi_disk_register() - register block devices
> > > - *
> > > - * U-Boot doesn't have a list of all online disk devices. So when running our
> > > - * EFI payload, we scan through all of the potentially available ones and
> > > - * store them in our object pool.
> > > +/*
> > > + * Create a handle for a disk partition
> > > *
> > > - * This function is called in efi_init_obj_list().
> > > + * @dev uclass device (UCLASS_PARTITION)
> > > *
> > > - * TODO(sjg at chromium.org): Actually with CONFIG_BLK, U-Boot does have this.
> > > - * Consider converting the code to look up devices as needed. The EFI device
> > > - * could be a child of the UCLASS_BLK block device, perhaps.
> > > + * Create an efi_disk object which is associated with @dev.
> > > + * The type of @dev must be UCLASS_PARTITION.
> > > *
> > > - * Return: status code
> > > + * @return 0 on success, -1 otherwise
> > > */
> > > -efi_status_t efi_disk_register(void)
> > > +static int efi_disk_create_part(struct udevice *dev)
> > > {
> > > + efi_handle_t parent;
> > > + struct blk_desc *desc;
> > > + const char *if_typename;
> > > + struct disk_part *part_data;
> > > + struct disk_partition *info;
> > > + unsigned int part;
> > > + int diskid;
> > > + struct efi_handler *handler;
> > > + struct efi_device_path *dp_parent;
> > > struct efi_disk_obj *disk;
> > > - int disks = 0;
> > > efi_status_t ret;
> > > +
> > > + if (dev_tag_get_ptr(dev_get_parent(dev), DM_TAG_EFI, (void **)&parent))
> > > + return -1;
> > > +
> > > + desc = dev_get_uclass_plat(dev_get_parent(dev));
> > > + if_typename = blk_get_if_type_name(desc->if_type);
> > > + diskid = desc->devnum;
> > > +
> > > + part_data = dev_get_uclass_plat(dev);
> > > + part = part_data->partnum;
> > > + info = &part_data->gpt_part_info;
> > > +
> > > + ret = efi_search_protocol(parent, &efi_guid_device_path, &handler);
> > > + if (ret != EFI_SUCCESS)
> > > + return -1;
> > > + dp_parent = (struct efi_device_path *)handler->protocol_interface;
> > > +
> > > + ret = efi_disk_add_dev(parent, dp_parent, if_typename, desc, diskid,
> > > + info, part, &disk);
> > > + if (ret != EFI_SUCCESS) {
> > > + log_err("Adding partition for %s failed\n", dev->name);
> > > + return -1;
> > > + }
> > > + if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) {
> > > + efi_free_pool(disk->dp);
> > > + efi_delete_handle(&disk->header);
> > > +
> > > + return -1;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/*
> > > + * Create efi_disk objects for a block device
> > > + *
> > > + * @dev uclass device (UCLASS_BLK)
> > > + *
> > > + * Create efi_disk objects for partitions as well as a raw disk
> > > + * which is associated with @dev.
> > > + * The type of @dev must be UCLASS_BLK.
> > > + * This function is expected to be called at EV_PM_POST_PROBE.
> > > + *
> > > + * @return 0 on success, -1 otherwise
> > > + */
> > > +static int efi_disk_probe(void *ctx, struct event *event)
> > > +{
> > > struct udevice *dev;
> > > + enum uclass_id id;
> > > + struct udevice *child;
> > > + int ret;
> > > - for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
> > > - uclass_next_device_check(&dev)) {
> > > - struct blk_desc *desc = dev_get_uclass_plat(dev);
> > > - const char *if_typename = blk_get_if_type_name(desc->if_type);
> > > + dev = event->data.dm.dev;
> > > + id = device_get_uclass_id(dev);
> > > - /* Add block device for the full device */
> > > - log_info("Scanning disk %s...\n", dev->name);
> > > - ret = efi_disk_add_dev(NULL, NULL, if_typename,
> > > - desc, desc->devnum, NULL, 0, &disk);
> > > - if (ret == EFI_NOT_READY) {
> > > - log_notice("Disk %s not ready\n", dev->name);
> > > - continue;
> > > - }
> > > - if (ret) {
> > > - log_err("ERROR: failure to add disk device %s, r = %lu\n",
> > > - dev->name, ret & ~EFI_ERROR_MASK);
> > > - continue;
> > > - }
> > > - disks++;
> > > + /* TODO: We won't support partitions in a partition */
> > > + if (id != UCLASS_BLK)
> > > + return 0;
> > > +
> > > + ret = efi_disk_create_raw(dev);
> > > + if (ret)
> > > + return -1;
> > > - /* Partitions show up as block devices in EFI */
> > > - disks += efi_disk_create_partitions(
> > > - &disk->header, desc, if_typename,
> > > - desc->devnum, dev->name);
> > > + device_foreach_child(child, dev) {
> > > + ret = efi_disk_create_part(child);
> > > + if (ret)
> > > + return -1;
> > > }
> > > - log_info("Found %d disks\n", disks);
> > > + return 0;
> > > +}
> > > +
> > > +efi_status_t efi_disk_init(void)
> > > +{
> > > + int ret;
> > > +
> > > + ret = event_register("efi_disk add", EVT_DM_POST_PROBE,
> > > + efi_disk_probe, NULL);
> > > + if (ret) {
> > > + log_err("Event registration for efi_disk add failed\n");
> > > + return EFI_OUT_OF_RESOURCES;
> > > + }
> > > return EFI_SUCCESS;
> > > }
> > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > > index de2f34bab537..250eeb2fcd6b 100644
> > > --- a/lib/efi_loader/efi_setup.c
> > > +++ b/lib/efi_loader/efi_setup.c
> > > @@ -198,9 +198,7 @@ static efi_status_t __efi_init_early(void)
> > > if (ret != EFI_SUCCESS)
> > > goto out;
> > > -#ifdef CONFIG_PARTITIONS
> > > - ret = efi_disk_register();
> > > -#endif
> > > + ret = efi_disk_init();
> > > out:
> > > return ret;
> > > }
> >
> > This change as commit a9bf024b2933bba0e23038892970a18b72dfaeb4 in master makes Amlogic S905X board libretech-cc to crash:
>
> Can you confirm that you do not use UEFI feature in your boot scenario here
> and yet you see a problem in UEFI subsystem?
>
> > Found U-Boot script /boot.scr
> > 449 bytes read in 2 ms (218.8 KiB/s)
> > ## Executing script at 08000000
> > 36500032 bytes read in 1554 ms (22.4 MiB/s)
> > 42873252 bytes read in 1829 ms (22.4 MiB/s)
> > 29092 bytes read in 4 ms (6.9 MiB/s)
> > ## Booting kernel from Legacy Image at 13000000 ...
> > Image Name:
> > Image Type: AArch64 Linux Kernel Image (uncompressed)
> > Data Size: 36499968 Bytes = 34.8 MiB
> > Load Address: 13000000
> > Entry Point: 13000000
> > Verifying Checksum ... OK
> > ## Loading init Ramdisk from Legacy Image at 06000000 ...
> > Image Name:
> > Image Type: AArch64 Linux RAMDisk Image (uncompressed)
> > Data Size: 42873188 Bytes = 40.9 MiB
> > Load Address: 00000000
> > Entry Point: 00000000
> > Verifying Checksum ... OK
> > ## Flattened Device Tree blob at 09000000
> > Booting using the fdt blob at 0x9000000
> > Loading Kernel Image
> > Loading Ramdisk to 7965b000, end 7bf3e164 ... OK
> > Loading Device Tree to 0000000079650000, end 000000007965a1a3 ... OK
> > "Synchronous Abort" handler, esr 0x96000004
> > elr: 0000000001065150 lr : 0000000001054b18 (reloc)
>
> Can you check where (functions) those two addresses (elr,lr) come from?
Do you still see the problem?
If so, please check my questions above.
-Takahiro Akashi
> > elr: 000000007dfb5150 lr : 000000007dfa4b18
> > x0 : d55d507f5dacfff5 x1 : 000000007dfbce48
> > x2 : 0000000000000010 x3 : 0000000000000000
> > x4 : 0000000000000000 x5 : d55d507f5dacfff5
> > x6 : 000000007bf55bb0 x7 : 0000000000000000
> > x8 : 0000000000000007 x9 : 0000000000000000
> > x10: 0000000000000178 x11: 000000007bf4211c
> > x12: 00000000000000a4 x13: 000000007bf420d8
> > x14: 0000000079650000 x15: 0000000000000020
> > x16: 000000007df83454 x17: 0000000000000000
> > x18: 000000007bf4ddb0 x19: 000000007af42040
> > x20: 000000007df50b20 x21: 000000007dfbce48
> > x22: 0000000000001000 x23: 000000007bf55b00
> > x24: 000000007dfdb910 x25: 0000000001000000
> > x26: 0000000001000000 x27: 0000000001000000
> > x28: 0000000000001000 x29: 000000007bf420d0
> >
> > Code: eb02009f 54000061 52800000 14000006 (386468a3)
>
> As far as my locally-built binary is concerned,
> the faulted instruction is located in memcpy().
>
> 000000000106xxxx <memcmp>:
> 106xxxx: aa0003e5 mov x5, x0
> 106xxxx: d2800004 mov x4, #0x0 // #0
> 106xxxx: eb02009f cmp x4, x2
> 106xxxx: 54000061 b.ne 10653d4 <memcmp+0x18> // b.any
> 106xxxx: 52800000 mov w0, #0x0 // #0
> 106xxxx: 14000006 b 10653e8 <memcmp+0x2c>
> 106xxxx: 386468a3 ldrb w3, [x5, x4]
>
> 'x5' seems to be bogus.
> The memory content might have been corrupted at this point.
>
> -Takahiro Akashi
>
> > Resetting CPU ...
> >
> > using libretech-cc_defconfig and the followin boot.scr:
> > setenv autoload no
> > setenv initrd_high 0xffffffff
> > setenv fdt_high 0xffffffff
> > load mmc 0:1 0x13000000 uImage
> > load mmc 0:1 0x6000000 ramdisk.cpio.gz.uboot
> > setenv initrd_size ${filesize}
> > load mmc 0:1 0x9000000 meson-gxl-s905x-libretech-cc.dtb
> > setenv bootargs 'console=ttyAML0,115200n8 root=/dev/ram0 console_msg_format=syslog earlycon ip=dhcp'
> > bootm 0x13000000 0x6000000 0x9000000
> >
> > bisect log:
> > # bad: [c387e62614713d0cc9e3ed022b86c9f320b02853] Merge branch '2022-05-11-Kconfig-cleanups-etc'
> > # good: [e4b6ebd3de982ae7185dbf689a030e73fd06e0d2] Prepare v2022.04
> > git bisect start 'c387e62614' 'v2022.04'
> > # good: [4228023eff7e94500137ce9e30687c00ffb4dd4f] led: bcm6753: Drop duplicate OF "label" property parsing
> > git bisect good 4228023eff7e94500137ce9e30687c00ffb4dd4f
> > # bad: [3fbdd485fd87ce0d4e1b747aea3c433646f4ced2] ARM: dts: sam9x60: Add pit64b node
> > git bisect bad 3fbdd485fd87ce0d4e1b747aea3c433646f4ced2
> > # good: [e50f66e364be80e02dd0834b84b830f3aade82ea] Merge https://source.denx.de/u-boot/custodians/u-boot-marvell
> > git bisect good e50f66e364be80e02dd0834b84b830f3aade82ea
> > # good: [9de612ae4ded53f742f5f99929c06d0839471ced] cmd: adc: Add support for storing ADC result in env variable
> > git bisect good 9de612ae4ded53f742f5f99929c06d0839471ced
> > # bad: [bc9da9fb50ac3ba7603487a0366d4db60b984812] Merge branch '2022-04-23-binman-updates'
> > git bisect bad bc9da9fb50ac3ba7603487a0366d4db60b984812
> > # bad: [3c809dfed757c37b92ff543c8f1c941407bafc2e] efi_loader: disk: not create BLK device for BLK(IF_TYPE_EFI_LOADER) devices
> > git bisect bad 3c809dfed757c37b92ff543c8f1c941407bafc2e
> > # good: [38f255b96085d2e50558a16256c87ffd885f2f7e] efi_loader: disk: compile efi_disk when CONFIG_BLK
> > git bisect good 38f255b96085d2e50558a16256c87ffd885f2f7e
> > # good: [8ff50227befdc778eb2cb239b778ed1ed843bf33] test: dm: add tests for tag support
> > git bisect good 8ff50227befdc778eb2cb239b778ed1ed843bf33
> > # good: [bf76031d19fa82041ae5ddc17214ec71858b0097] dm: blk: add a device-probe hook for scanning disk partitions
> > git bisect good bf76031d19fa82041ae5ddc17214ec71858b0097
> > # bad: [a9bf024b2933bba0e23038892970a18b72dfaeb4] efi_loader: disk: a helper function to create efi_disk objects from udevice
> > git bisect bad a9bf024b2933bba0e23038892970a18b72dfaeb4
> > # good: [a57ad20d07e82f9ddbbdf981c8f8dd5368b225e4] efi_loader: split efi_init_obj_list() into two stages
> > git bisect good a57ad20d07e82f9ddbbdf981c8f8dd5368b225e4
> > # first bad commit: [a9bf024b2933bba0e23038892970a18b72dfaeb4] efi_loader: disk: a helper function to create efi_disk objects from udevice
> >
> >
> > Neil
> >
More information about the U-Boot
mailing list