[U-Boot] [PATCH v2 2/2] efi_loader: device_path: show a host device in understandable form

[Heinrich Schuchardt]

On 10/3/19 8:56 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> On Fri, Sep 13, 2019 at 09:20:53AM +0900, AKASHI Takahiro wrote:
>> On Thu, Sep 12, 2019 at 11:50:04AM +0200, Heinrich Schuchardt wrote:
>>> On 9/12/19 11:07 AM, AKASHI Takahiro wrote:
>>>> On Thu, Sep 12, 2019 at 09:59:01AM +0200, Heinrich Schuchardt wrote:
>>>>> On 9/12/19 6:52 AM, AKASHI Takahiro wrote:
>>>>>> It would be better to give a user-friendly text to a host device
>>>>>> on sandbox instead of just dumping its guid.
>>>>>>
>>>>>> => host bind 0 /opt/disk/uboot_sandbox_fat.img
>>>>>> => efi devices
>>>>>> Device           Device Path
>>>>>> ================ ====================
>>>>>> 0000000015c1f3a0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
>>>>>> 0000000015c20f00 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Hostdev(0)
>>>>>>
>>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
>>>>>> ---
>>>>>>   lib/efi_loader/efi_device_path_to_text.c | 5 +++++
>>>>>>   1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c
>>>>>> index 96fd08971b73..40a06b70e08a 100644
>>>>>> --- a/lib/efi_loader/efi_device_path_to_text.c
>>>>>> +++ b/lib/efi_loader/efi_device_path_to_text.c
>>>>>> @@ -62,6 +62,11 @@ static char *dp_hardware(char *s, struct efi_device_path *dp)
>>>>>>   	case DEVICE_PATH_SUB_TYPE_VENDOR: {
>>>>>>   		struct efi_device_path_vendor *vdp =
>>>>>>   			(struct efi_device_path_vendor *)dp;
>>>>>> +#ifdef CONFIG_SANDBOX
>>>>>> +		if (!guidcmp(&vdp->guid, &efi_guid_host_dev))
>>>>>> +			s += sprintf(s, "Hostdev(%d)", vdp->vendor_data[0]);
>>>>>
>>>>> This does not conform to the UEFI spec.
>>>>
>>>> Okay, so I'd like to change the format again.
>>>> Instead of 'Vendor' subtype, use 'Controller' subtype.
>>>> This way, the example above can be seen as:
>>>>
>>>> 0000000015c1f3a0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
>>>> 0000000015c20f00 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Ctrl(0)
>>>>
>>>> Looks nice, doesn't it?
>>>
>>> A controller is a handle that bears the EFI_DRIVER_BINDING_PROTOCOL.
>>
>> To my best knowledge, I don't find such a definition in the specification.
>
> Do you have any comments?
>
>>
>>>>> The purpose of the sandbox is testing. What would make sense to me is
>>>>> checking in a Python test that The VenHw() output contains the GUID and
>>>>> the drive number.
>>
>> For *that* reason, we don't have to stick to strict *standardized*
>> form for Sandbox host device. It would never cause any problems
>> as such a text would never come up on any other platforms.
>
> Any comments?

As the Sandbox host device is vendor specific VenHw() is a good fit.
I see no benefit in changing the current solution and would prefer not
to make this code more complicated.

Best regards

Heinrich

>
> -Takahiro Akashi
>>
>>
>>>>>
>>>>> Best regards
>>>>>
>>>>> Heinrich
>>>>>
>>>>>> +		else
>>>>>> +#endif
>>>>>>   		s += sprintf(s, "VenHw(%pUl)", &vdp->guid);
>>>>>>   		break;
>>>>>>   	}
>>>>>>
>>>>>
>>>>
>>>
>