[U-Boot] [PATCH 2/2] efi_loader: device_path: support Sandbox's "host" devices

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Heinrich,

Apologies for having not replied.

On Fri, Aug 23, 2019 at 08:09:45PM +0200, Heinrich Schuchardt wrote:
> On 8/23/19 1:34 AM, AKASHI Takahiro wrote:
> >On Thu, Aug 22, 2019 at 08:19:24PM +0200, Heinrich Schuchardt wrote:
> >>On 8/22/19 10:54 AM, AKASHI Takahiro wrote:
> >>>Sandbox's "host" devices are currently described as UCLASS_ROOT udevice
> >>>with DEV_IF_HOST block device. As the current implementation of
> >>>efi_device_path doesn't support such a type, any "host" device
> >>>on sandbox cannot be seen as a distinct object.
> >>>
> >>>For example,
> >>>   => host bind 0 /foo/disk.img
> >>>
> >>>   => efi devices
> >>>   Scanning disk host0...
> >>>   Found 1 disks
> >>>   Device           Device Path
> >>>   ================ ====================
> >>>   0000000015c19970 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> >>>   0000000015c19d70 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> >>>
> >>>   => efi dh
> >>>   Handle           Protocols
> >>>   ================ ====================
> >>>   0000000015c19970 Device Path, Device Path To Text, Device Path Utilities, Unicode Collation 2, HII String, HII Database, HII Config Routing
> >>>   0000000015c19ba0 Driver Binding
> >>>   0000000015c19c10 Simple Text Output
> >>>   0000000015c19c80 Simple Text Input, Simple Text Input Ex
> >>>   0000000015c19d70 Block IO, Device Path, Simple File System
> >>>
> >>>As you can see here, efi_root (0x0000000015c19970) and host0 device
> >>>(0x0000000015c19d70) has the same representation of device path.
> >>>
> >>>This is not only inconvenient, but also confusing since two different
> >>>efi objects are associated with the same device path and
> >>>efi_dp_find_obj() will possibly return a wrong result.
> >>>
> >>>Solution:
> >>>Each "host" device should be given an additional device path node
> >>>of Logical Unit Number(LUN) to make it distinguishable. The result
> >>>would be:
> >>>   Device           Device Path
> >>>   ================ ====================
> >>>   0000000015c19970 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> >>>   0000000015c19d70 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/LUN(0)
> >>>
> >>>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>---
> >>>  lib/efi_loader/efi_device_path.c | 22 ++++++++++++++++++++++
> >>>  1 file changed, 22 insertions(+)
> >>>
> >>>diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> >>>index eeeb80683607..565bb6888fe1 100644
> >>>--- a/lib/efi_loader/efi_device_path.c
> >>>+++ b/lib/efi_loader/efi_device_path.c
> >>>@@ -12,6 +12,7 @@
> >>>  #include <mmc.h>
> >>>  #include <efi_loader.h>
> >>>  #include <part.h>
> >>>+#include <sandboxblockdev.h>
> >>>  #include <asm-generic/unaligned.h>
> >>>
> >>>  /* template END node: */
> >>>@@ -422,6 +423,10 @@ static unsigned dp_size(struct udevice *dev)
> >>>
> >>>  	switch (dev->driver->id) {
> >>>  	case UCLASS_ROOT:
> >>>+#ifdef CONFIG_SANDBOX
> >>>+		/* stop traversing parents at this point: */
> >>>+		return sizeof(ROOT) + sizeof(struct efi_device_path_lun);
> >>>+#endif
> >>>  	case UCLASS_SIMPLE_BUS:
> >>>  		/* stop traversing parents at this point: */
> >>>  		return sizeof(ROOT);
> >>>@@ -505,6 +510,23 @@ static void *dp_fill(void *buf, struct udevice *dev)
> >>>  #ifdef CONFIG_BLK
> >>>  	case UCLASS_BLK:
> >>>  		switch (dev->parent->uclass->uc_drv->id) {
> >>>+#ifdef CONFIG_SANDBOX
> >>>+		case UCLASS_ROOT: {
> >>>+			/* stop traversing parents at this point: */
> >>>+			struct efi_device_path_vendor *vdp = buf;
> >>>+			struct efi_device_path_lun *dp;
> >>>+			struct host_block_dev *host_dev = dev_get_platdata(dev);
> >>>+			struct blk_desc *desc = dev_get_uclass_platdata(dev);
> >>>+
> >>>+			*vdp++ = ROOT;
> >>>+			dp = (struct efi_device_path_lun *)vdp;
> >>>+			dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> >>>+			dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_LUN;
> >>>+			dp->dp.length = sizeof(*dp);
> >>>+			dp->logical_unit_number = desc->devnum;
> >>>+			return ++dp;
> >>>+			}
> >>>+#endif
> >>>  #ifdef CONFIG_IDE
> >>>  		case UCLASS_IDE: {
> >>>  			struct efi_device_path_atapi *dp =
> >>>
> >>
> >>Hello Takahiro,
> >>
> >>thank you for pointing out that currently we generate the same device
> >>path twice. This for sure is something we need to fix.
> >>
> >>In the table below you will find the device tree for
> >>
> >>./u-boot -d arch/sandbox/dts/test.dtb
> >>
> >>In the device tree I see exactly one root node. I see no device called
> >>host0.
> >
> >"Host0" or whatever other host device is a pseudo device which will
> >be dynamically created, like other hot-pluggable devices, by "host bind"
> >command on sandbox U-Boot. So it won't appear in device tree.
> >
> >-Takahiro Akashi
> 
> Thank you for the explanation.
> 
> The host device is shown in the device tree:

"device tree" is a confusing term. Is "dm tree" better?


> make sandbox_defconfig
> make
> ./u-boot
> => host bind 0 ../sct-amd64.img
> => dm tree
>  Class     Index  Probed  Driver                Name
> -----------------------------------------------------------
>  root         0  [ + ]   root_driver           root_driver
> <snip />
>  blk          0  [ + ]   sandbox_host_blk      `-- host0
> 
> I guess the device node for host0 should be of type "Vendor-Defined
> Media Device Path". The field "Vendor Defined Data" can be used to
> differentiate between host0 - host3.

Okay, agreed.
But this will also create another gap between "dm tree"
and device path representation in UEFI.

> But for MMC, USB, SATA, and SCSI devices you would want to use another
> node type.

There is no issue that I'm aware of on those devices.

> The code in your patch should not depend on CONFIG_SANDBOX. Instead in
> the driver model you can simply walk up the device tree up to its root
> node and assign a device path node to each node on the device tree path
> according to its UCLASS (UCLASS_BLK here) and according to its interface
> type (IF_TYPE_HOST here) in the case of block devices.

I don't get your point.
IICU, what you claim above is the exact same as my patch does.
The issue is not 'walking dm tree,' which is already done
by efi_disk_register(), but adding an extra "device path" to
host device.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich