[PATCH 1/1] efi_loader: device paths for special block devices
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.
The current implementation fails to create unique device paths for block devices based on other uclasses like UCLASS_PVBLOCK.
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@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); } #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 =
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@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@linaro.org
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@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;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)) {@@ -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@linaro.org
participants (2)
-
Heinrich Schuchardt -
Ilias Apalodimas