[PATCH 1/1] efi_loader: device paths for special block devices
Heinrich Schuchardt
heinrich.schuchardt at canonical.com
Fri Jul 28 11:34:37 CEST 2023
On 28.07.23 11:14, Ilias Apalodimas wrote:
> 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?
Yes, the idea was that the default will catch all block devices that we
cannot map to anything else but VenHw(). This includes UCLASS_PVBLOCK.
Best regards
Heinrich
>
>>
>> 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