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 <[email protected]> > --- > 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 <[email protected]>
