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

[Heinrich Schuchardt]

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 at 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:

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.

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

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.

Best regards

Heinrich