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 <[email protected]> > >>>--- > >>> 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
