[PATCH 1/1] efi_loader: device paths for special block devices
Ilias Apalodimas
ilias.apalodimas at linaro.org
Fri Jul 28 11:14:28 CEST 2023
Hi Heinrich,
On Fri, Jul 21, 2023 at 12:03:46AM +0200, Heinrich Schuchardt wrote:
> The UEFI specification does not provide node types matching UCLASS_BLKMAP,
> UCLASS_HOST, UCLASS_VIRTIO block devices.
>
> The current implementation uses VenHw() nodes with uclass specific GUIDs
> and a single byte for the device number appended. This leads to unaligned
> integers is succeeding device path nodes.
in succeeding
>
> The current implementation fails to create unique device paths for block
> devices based on other uclasses like UCLASS_PVBLOCK.
Is this fixed as well with this patch because we have a sane default switch
now?
>
> Let's use a VenHw() node with the U-Boot GUID with a length dividable by
> four and encoding blkdesc->uclass_id as well as blkdesc->devnum.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> ---
> lib/efi_loader/efi_device_path.c | 114 ++++++-------------------------
> 1 file changed, 21 insertions(+), 93 deletions(-)
>
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index 19e8861ef4..5b050fa17c 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -22,16 +22,6 @@
> #include <asm-generic/unaligned.h>
> #include <linux/compat.h> /* U16_MAX */
>
> -#ifdef CONFIG_BLKMAP
> -const efi_guid_t efi_guid_blkmap_dev = U_BOOT_BLKMAP_DEV_GUID;
> -#endif
> -#ifdef CONFIG_SANDBOX
> -const efi_guid_t efi_guid_host_dev = U_BOOT_HOST_DEV_GUID;
> -#endif
> -#ifdef CONFIG_VIRTIO_BLK
> -const efi_guid_t efi_guid_virtio_dev = U_BOOT_VIRTIO_DEV_GUID;
> -#endif
> -
> /* template END node: */
> const struct efi_device_path END = {
> .type = DEVICE_PATH_TYPE_END,
> @@ -524,43 +514,15 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev)
> return dp_size(dev->parent) +
> sizeof(struct efi_device_path_nvme);
> #endif
> -#ifdef CONFIG_SANDBOX
> - case UCLASS_HOST:
> - /*
> - * Sandbox's host device will be represented
> - * as vendor device with extra one byte for
> - * device number
> - */
> - return dp_size(dev->parent)
> - + sizeof(struct efi_device_path_vendor) + 1;
> -#endif
> #ifdef CONFIG_USB
> case UCLASS_MASS_STORAGE:
> return dp_size(dev->parent)
> + sizeof(struct efi_device_path_controller);
> -#endif
> -#ifdef CONFIG_VIRTIO_BLK
> - case UCLASS_VIRTIO:
> - /*
> - * Virtio devices will be represented as a vendor
> - * device node with an extra byte for the device
> - * number.
> - */
> - return dp_size(dev->parent)
> - + sizeof(struct efi_device_path_vendor) + 1;
> -#endif
> -#ifdef CONFIG_BLKMAP
> - case UCLASS_BLKMAP:
> - /*
> - * blkmap devices will be represented as a vendor
> - * device node with an extra byte for the device
> - * number.
> - */
> - return dp_size(dev->parent)
> - + sizeof(struct efi_device_path_vendor) + 1;
> #endif
> default:
> - return dp_size(dev->parent);
> + /* UCLASS_BLKMAP, UCLASS_HOST, UCLASS_VIRTIO */
> + return dp_size(dev->parent) +
> + sizeof(struct efi_device_path_udevice);
We are handling these for now , but this ends up being a catch all for all
UCLASS devices that arent explicitly handled but I guess this is ok?
> }
> #if defined(CONFIG_MMC)
> case UCLASS_MMC:
> @@ -608,53 +570,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev)
> }
> #endif
> case UCLASS_BLK:
> - switch (dev->parent->uclass->uc_drv->id) {
> -#ifdef CONFIG_BLKMAP
> - case UCLASS_BLKMAP: {
> - struct efi_device_path_vendor *dp;
> - struct blk_desc *desc = dev_get_uclass_plat(dev);
> -
> - dp = dp_fill(buf, dev->parent);
> - dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
> - dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR;
> - dp->dp.length = sizeof(*dp) + 1;
> - memcpy(&dp->guid, &efi_guid_blkmap_dev,
> - sizeof(efi_guid_t));
> - dp->vendor_data[0] = desc->devnum;
> - return &dp->vendor_data[1];
> - }
> -#endif
> -#ifdef CONFIG_SANDBOX
> - case UCLASS_HOST: {
> - /* stop traversing parents at this point: */
> - struct efi_device_path_vendor *dp;
> - struct blk_desc *desc = dev_get_uclass_plat(dev);
> -
> - dp = dp_fill(buf, dev->parent);
> - dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
> - dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR;
> - dp->dp.length = sizeof(*dp) + 1;
> - memcpy(&dp->guid, &efi_guid_host_dev,
> - sizeof(efi_guid_t));
> - dp->vendor_data[0] = desc->devnum;
> - return &dp->vendor_data[1];
> - }
> -#endif
> -#ifdef CONFIG_VIRTIO_BLK
> - case UCLASS_VIRTIO: {
> - struct efi_device_path_vendor *dp;
> - struct blk_desc *desc = dev_get_uclass_plat(dev);
> -
> - dp = dp_fill(buf, dev->parent);
> - dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
> - dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR;
> - dp->dp.length = sizeof(*dp) + 1;
> - memcpy(&dp->guid, &efi_guid_virtio_dev,
> - sizeof(efi_guid_t));
> - dp->vendor_data[0] = desc->devnum;
> - return &dp->vendor_data[1];
> - }
> -#endif
> + switch (device_get_uclass_id(dev->parent)) {
> #ifdef CONFIG_IDE
> case UCLASS_IDE: {
> struct efi_device_path_atapi *dp =
> @@ -744,12 +660,24 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev)
> return &dp[1];
> }
> #endif
> - default:
> - debug("%s(%u) %s: unhandled parent class: %s (%u)\n",
> - __FILE__, __LINE__, __func__,
> - dev->name, dev->parent->uclass->uc_drv->id);
> - return dp_fill(buf, dev->parent);
> + default: {
> + /* UCLASS_BLKMAP, UCLASS_HOST, UCLASS_VIRTIO */
> + struct efi_device_path_udevice *dp;
> + struct blk_desc *desc = dev_get_uclass_plat(dev);
> +
> + dp = dp_fill(buf, dev->parent);
> + dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
> + dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR;
> + dp->dp.length = sizeof(*dp);
> + memcpy(&dp->guid, &efi_u_boot_guid,
> + sizeof(efi_guid_t));
> + dp->uclass_id = (UCLASS_BLK & 0xffff) |
> + (desc->uclass_id << 16);
> + dp->dev_number = desc->devnum;
> +
> + return &dp[1];
> }
> + }
> #if defined(CONFIG_MMC)
> case UCLASS_MMC: {
> struct efi_device_path_sd_mmc_path *sddp =
> --
> 2.40.1
>
In any case the patch LGTM
Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
More information about the U-Boot
mailing list