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

Wed Sep 11 17:22:57 UTC 2019

On 9/11/19 8:35 AM, AKASHI Takahiro wrote:
> 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 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:
>
> "device tree" is a confusing term. Is "dm tree" better?

The README calls it Live Device Tree.
./doc/driver-model/livetree.rst

But yes I mean the tree of the driver model.

>
>
>> 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.

Previously you suggested using the logical unit class (LUN). I think a
vendor defined node is more appropriate.

Commit 8254f8feb71a93a4d87aa68d900660ef445d44cd
efi_loader: correct text conversion for vendor DP

ensures that you can print out the extra data identifying the individual
host number.

>
>> 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.

Sorry, the mistake is on my side.

Best regards

Heinrich