diff mbox series

[v5,07/12] efi_loader: disk: a helper function to create efi_disk objects from udevice

Message ID 20220419010517.47175-8-takahiro.akashi@linaro.org
State New
Headers show
Series efi_loader: more tightly integrate UEFI disks to driver model | expand

Commit Message

AKASHI Takahiro April 19, 2022, 1:05 a.m. UTC
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@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(-)

Comments

Johan Jonker April 30, 2022, 6:44 p.m. UTC | #1
On 4/19/22 03:05, AKASHI Takahiro wrote:

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

Hi,

The use of this select without EVENT leads to this warning:

WARNING: unmet direct dependencies detected for EVENT_DYNAMIC
  Depends on [n]: EVENT [=n]
  Selected by [y]:
  - EFI_LOADER [=y] && OF_LIBFDT [=y] && (ARM [=y] && (SYS_CPU
[=armv7]=arm1136 || SYS_CPU [=armv7]=arm1176 || SYS_CPU [=armv7]=armv7
|| SYS_CPU [=armv7]=armv8) || X86 [=n] || RISCV [=n] || SANDBOX [=n]) &&
(!EFI_STUB [=n] || !X86_64 [=n] || EFI_STUB_64BIT [=n]) && (!EFI_STUB
[=n] || !X86 [=n] || X86_64 [=n] || EFI_STUB_32BIT [=n]) && BLK [=y] &&
(DM_ETH [=n] || !NET [=n]) && !EFI_APP [=n]

Previously there was no need for events on rk3066.
Is that normal behavior or should we include "select EVENT" here as well?

Johan

>  	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"
Neil Armstrong May 18, 2022, 9:23 a.m. UTC | #2
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@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@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:
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)
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)
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
AKASHI Takahiro May 19, 2022, 5:04 a.m. UTC | #3
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@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@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?

> 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
>
AKASHI Takahiro June 1, 2022, 12:25 a.m. UTC | #4
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@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@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
> >
Fabio Estevam June 13, 2022, 7:54 p.m. UTC | #5
On Wed, May 18, 2022 at 6:23 AM Neil Armstrong <narmstrong@baylibre.com> wrote:

> This change as commit a9bf024b2933bba0e23038892970a18b72dfaeb4 in master makes Amlogic S905X board libretech-cc to crash:

This change also causes issues on a kontron-sl-mx8mm and imx8mn ddr3 evk boards.

The problem on these boards is that serial console garbage is seen:

Net:   eth0: ethernet@30be0000
Hit any key to stop autoboot:  0
u-boot=> [25;88R
Unknown command '[25' - try 'help'
Unknown command '88R' - try 'help'

These extraneous characters stop the boot process.

As a workaround, I disabled CONFIG_EFI_LOADER, and then the serial
garbage does not happen and the board can boot normally.

Heiko confirmed the same behavior on an imx8mn evk ddr3 board.
Tom Rini June 13, 2022, 8:10 p.m. UTC | #6
On Mon, Jun 13, 2022 at 04:54:45PM -0300, Fabio Estevam wrote:
> On Wed, May 18, 2022 at 6:23 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> 
> > This change as commit a9bf024b2933bba0e23038892970a18b72dfaeb4 in master makes Amlogic S905X board libretech-cc to crash:
> 
> This change also causes issues on a kontron-sl-mx8mm and imx8mn ddr3 evk boards.
> 
> The problem on these boards is that serial console garbage is seen:
> 
> Net:   eth0: ethernet@30be0000
> Hit any key to stop autoboot:  0
> u-boot=> [25;88R
> Unknown command '[25' - try 'help'
> Unknown command '88R' - try 'help'
> 
> These extraneous characters stop the boot process.
> 
> As a workaround, I disabled CONFIG_EFI_LOADER, and then the serial
> garbage does not happen and the board can boot normally.
> 
> Heiko confirmed the same behavior on an imx8mn evk ddr3 board.

What happens if you increase logging so that you can see other errors
that happen as it feels like we're probably overflowing some buffer or
something along the way.
Mark Kettenis June 13, 2022, 10:21 p.m. UTC | #7
> Date: Mon, 13 Jun 2022 16:10:57 -0400
> From: Tom Rini <trini@konsulko.com>
> 
> On Mon, Jun 13, 2022 at 04:54:45PM -0300, Fabio Estevam wrote:
> > On Wed, May 18, 2022 at 6:23 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> > 
> > > This change as commit a9bf024b2933bba0e23038892970a18b72dfaeb4 in master makes Amlogic S905X board libretech-cc to crash:
> > 
> > This change also causes issues on a kontron-sl-mx8mm and imx8mn ddr3 evk boards.
> > 
> > The problem on these boards is that serial console garbage is seen:
> > 
> > Net:   eth0: ethernet@30be0000
> > Hit any key to stop autoboot:  0
> > u-boot=> [25;88R
> > Unknown command '[25' - try 'help'
> > Unknown command '88R' - try 'help'
> > 
> > These extraneous characters stop the boot process.

This looks like the reply the terminal sends in response to the "Query
cursor position" command that is sent in
lib/efi_loader/efi_console.c:query_console_serial().

It might be worth trying to increase the timeout in term_get_char().

If that doesn't help, it probably means the initial ESC character gets
lost because the serial port has a very small (or no) FIFO and u-boot
isn't reading characters fast enough.  If that is the case, maybe we
we should make the query_console_serial() call optional and only
enable it on SoCs that have a large enough FIFO for this to be safe.

Cheers,

Mark
Fabio Estevam June 13, 2022, 10:48 p.m. UTC | #8
Hi Mark,

On Mon, Jun 13, 2022 at 7:21 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:

> This looks like the reply the terminal sends in response to the "Query
> cursor position" command that is sent in
> lib/efi_loader/efi_console.c:query_console_serial().
>
> It might be worth trying to increase the timeout in term_get_char().

I did as you suggested and increased the timeout from 100ms to 1000ms.

After that, I no longer see the problem, thanks!

Would this be an acceptable fix?
Heinrich Schuchardt June 13, 2022, 11:39 p.m. UTC | #9
Am 14. Juni 2022 00:48:01 MESZ schrieb Fabio Estevam <festevam@gmail.com>:
>Hi Mark,
>
>On Mon, Jun 13, 2022 at 7:21 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
>> This looks like the reply the terminal sends in response to the "Query
>> cursor position" command that is sent in
>> lib/efi_loader/efi_console.c:query_console_serial().
>>
>> It might be worth trying to increase the timeout in term_get_char().
>
>I did as you suggested and increased the timeout from 100ms to 1000ms.
>
>After that, I no longer see the problem, thanks!
>
>Would this be an acceptable fix?

Slowing down every boot without connected serial by 1000ms seems to be a bad idea.

What baud rate are you using?

What terminal software are you using giving the late response?

Does printf() return before the last byte is sent by the serial IO transfer buffer? What size has that buffer?

I suggest that the EFI console should not query the screen size before starting the first EFI binary.

Best regards

Heinrich
AKASHI Takahiro June 13, 2022, 11:39 p.m. UTC | #10
On Mon, Jun 13, 2022 at 07:48:01PM -0300, Fabio Estevam wrote:
> Hi Mark,
> 
> On Mon, Jun 13, 2022 at 7:21 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> > This looks like the reply the terminal sends in response to the "Query
> > cursor position" command that is sent in
> > lib/efi_loader/efi_console.c:query_console_serial().
> >
> > It might be worth trying to increase the timeout in term_get_char().
> 
> I did as you suggested and increased the timeout from 100ms to 1000ms.
> 
> After that, I no longer see the problem, thanks!

So I hope that my patch has broken nothing.

-Takahiro Akashi

> Would this be an acceptable fix?
Fabio Estevam June 13, 2022, 11:54 p.m. UTC | #11
Hi Heinrich,

On Mon, Jun 13, 2022 at 8:39 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> Slowing down every boot without connected serial by 1000ms seems to be a bad idea.
>
> What baud rate are you using?

115200.

> What terminal software are you using giving the late response?

I will check. I am accessing the board remotely.

Heiko also sees the problem. Maybe he could share what terminal
software he uses.

> Does printf() return before the last byte is sent by the serial IO transfer buffer? What size has that buffer?

Looks like I would need to instrument the code to answer this.

> I suggest that the EFI console should not query the screen size before starting the first EFI binary.

Could you please propose I patch doing this?

To be honest, I don't need EFI, but it gets automatically selected,
and that's why I see the problem.

For the moment, I am disabling CONFIG_EFI_LOADER as a workaround.

I can help to test patches, but I am not familiar with EFI to propose a fix.

Thanks,

Fabio Estevam
Heiko Thiery June 14, 2022, 3:59 a.m. UTC | #12
Hi,

Am Di., 14. Juni 2022 um 01:54 Uhr schrieb Fabio Estevam <festevam@gmail.com>:
>
> Hi Heinrich,
>
> On Mon, Jun 13, 2022 at 8:39 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> > Slowing down every boot without connected serial by 1000ms seems to be a bad idea.
> >
> > What baud rate are you using?
>
> 115200.
>
> > What terminal software are you using giving the late response?
>
> I will check. I am accessing the board remotely.
>
> Heiko also sees the problem. Maybe he could share what terminal
> software he uses.

I use ser2net and connect with the linux telnet client.

> > Does printf() return before the last byte is sent by the serial IO transfer buffer? What size has that buffer?
>
> Looks like I would need to instrument the code to answer this.
>
> > I suggest that the EFI console should not query the screen size before starting the first EFI binary.
>
> Could you please propose I patch doing this?
>
> To be honest, I don't need EFI, but it gets automatically selected,
> and that's why I see the problem.
>
> For the moment, I am disabling CONFIG_EFI_LOADER as a workaround.
>
> I can help to test patches, but I am not familiar with EFI to propose a fix.
>
> Thanks,
>
> Fabio Estevam
Viacheslav Aug. 24, 2022, 9 a.m. UTC | #13
I has similar problem on axg meson64. Seems problem exists at least on 
1Gb RAM devices, my device with 2 GB RAM did not hit this error.


19.05.2022 8:04, 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@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@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?
> 
>> 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.

Same place:

"Synchronous Abort" handler, esr 0x96000004
elr: 000000000105ff48 lr : 00000000010504d4 (reloc)
elr: 000000003ffa4f48 lr : 000000003ff954d4
x0 : 5695c9ebf0a7d8b1 x1 : 000000003ffac700
x2 : 0000000000000010 x3 : 0000000000000000
x4 : 0000000000000000 x5 : 5695c9ebf0a7d8b1
x6 : 000000003df4e3b0 x7 : 0000000000000007
x8 : 0000000000000000 x9 : 0000000000000008
x10: 0000000000008030 x11: 000000003df3496c
x12: 0000000000007fa8 x13: 000000003df34928
x14: 000000003ce18000 x15: 0000000000000020
x16: 000000003ff75d74 x17: 0000000000000000
x18: 000000003df42dc0 x19: 000000003cf34040
x20: 000000003ff45b20 x21: 000000003ffac700
x22: 0000000000000300 x23: 000000003df4e300
x24: 000000003ffca5e8 x25: 0000000005300000
x26: 0000000000300000 x27: 0000000000000000
x28: 0000000000000300 x29: 000000003df34930

Code: eb02009f 54000061 52800000 14000006 (386468a3)
000000000105ff30 <memcmp>:
  105ff30:       aa0003e5        mov     x5, x0
  105ff34:       d2800004        mov     x4, #0x0 // #0
  105ff38:       eb02009f        cmp     x4, x2
  105ff3c:       54000061        b.ne    105ff48 <memcmp+0x18>  // b.any
  105ff40:       52800000        mov     w0, #0x0 // #0
  105ff44:       14000006        b       105ff5c <memcmp+0x2c>
*105ff48:       386468a3        ldrb    w3, [x5, x4]
  105ff4c:       38646820        ldrb    w0, [x1, x4]

I don't know how to get the call stack from this information.
What else can I do to investigate the problem?
Viacheslav Aug. 24, 2022, 9:29 a.m. UTC | #14
Config for jethub_j100 did't contain any EFI* options. But build .config 
has these lines:
CONFIG_BOOTMETH_EFILOADER=y
CONFIG_CMD_BOOTEFI=y
CONFIG_CMD_BOOTEFI_HELLO_COMPILE=y
CONFIG_EFI_PARTITION=y
CONFIG_EFI_LOADER=y
CONFIG_CMD_BOOTEFI_BOOTMGR=y
CONFIG_EFI_SETUP_EARLY=y
CONFIG_EFI_VARIABLE_FILE_STORE=y
CONFIG_EFI_GET_TIME=y
CONFIG_EFI_DEVICE_PATH_TO_TEXT=y
CONFIG_EFI_DEVICE_PATH_UTIL=y
CONFIG_EFI_DT_FIXUP=y
CONFIG_EFI_LOADER_HII=y
CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2=y
CONFIG_EFI_UNICODE_CAPITALIZATION=y
CONFIG_EFI_HAVE_RUNTIME_RESET=y
CONFIG_EFI_RNG_PROTOCOL=y
CONFIG_EFI_LOAD_FILE2_INITRD=y

If I set it all to "n" bug dissapears and board boots normally.

24.08.2022 12:00, Vyacheslav wrote:
> I has similar problem on axg meson64. Seems problem exists at least on 
> 1Gb RAM devices, my device with 2 GB RAM did not hit this error.
diff mbox series

Patch

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@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;
 }