Re: [RFC 3/3] cmd: efidebug: handle booting from removable media

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

On 11/10/21 09:11, Heinrich Schuchardt wrote:
> 
> 
> On 11/10/21 07:24, AKASHI Takahiro wrote:
>> On Tue, Nov 09, 2021 at 10:50:37AM +0100, Heinrich Schuchardt wrote:
>>> On 11/9/21 02:32, AKASHI Takahiro wrote:
>>>> Support for booting from removable media is now added to UEFI boot
>>>> manager. Here we should modify efidebug command in order to define
>>>> a proper "BootXXXX" variable.
>>>>
>>>> With this patch applied, you will be able to specify the boot order,
>>>> usb and scsi:
>>>
>>> I guess for a removable device this should work even if the device is 
>>> not
>>> present. But currently:
>>>
>>> => efidebug boot add -b 1000 USB_present usb 0:1 EFI/BOOT/BOOTARM.EFI
>>> => efidebug boot add -b 1000 USB_not_present usb 1:1 
>>> EFI/BOOT/BOOTARM.EFI
>>> Cannot create device path for "usb 1:1"
>>
>> # In the second, I don't expect you to specify the file path,
>> # "EFI/BOOT/BOOTARM.EFI", for removable media support.
>>
>> Yes, I have been aware of this issue but it is not this-patch specific
>> but an existing issue as you clearly mentioned above.
>>
>>> A media device path like:
>>>
>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0)/HD(1,MBR,0x0c449046,0x800,0x800) 
>>>
>>>
>>> is not very helpful because the next device that you insert may have a
>>> different location of the ESP partition.
>>>
>>> I think you should store
>>>
>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0) 
>>>
>>
>> Apparently it is promising but actually not because
>> "UsbClass(0x781,0x5571,0x0,0x0,0x0)" contains Vendor ID and
>> Product ID which can only be retrieved from a real device attached
>> to the board.
> 
> 0x781,0x5571 relates to
> Vendor: SanDisk Corp.
> Device: Cruzer Fit
> 
> This is how Linux sees my OrangePi PC:
> 
> $ lsusb
> Bus 009 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 007 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 008 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 006 Device 002: ID 0781:5571 SanDisk Corp. Cruzer Fit
> Bus 006 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 005 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> 
> The bus numbering seems not to be stable:
> 
> $ lsusb
> Bus 009 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 008 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 007 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 005 Device 002: ID 0781:5571 SanDisk Corp. Cruzer Fit
> Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 006 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> 
> 
> And this is in U-Boot:
> 
> usb           0  [ + ]   ehci_generic          |   |-- usb@1c1a000
> usb_hub       0  [ + ]   usb_hub               |   |   `-- usb_hub
> usb           1  [ + ]   ohci_generic          |   |-- usb@1c1a400
> usb_hub       1  [ + ]   usb_hub               |   |   `-- usb_hub
> usb           2  [ + ]   ehci_generic          |   |-- usb@1c1b000
> usb_hub       2  [ + ]   usb_hub               |   |   `-- usb_hub
> usb           3  [ + ]   ohci_generic          |   |-- usb@1c1b400
> usb_hub       3  [ + ]   usb_hub               |   |   `-- usb_hub
> usb           4  [ + ]   ehci_generic          |   |-- usb@1c1c000
> usb_hub       4  [ + ]   usb_hub               |   |   `-- usb_hub
> usb_mass_s    0  [ + ]   usb_mass_storage      |   |       `-- 
> usb_mass_storage
> blk           1  [   ]   usb_storage_blk       |   |           `-- 
> usb_mass_storage.lun0
> usb           5  [ + ]   ohci_generic          |   |-- usb@1c1c400
> usb_hub       5  [ + ]   usb_hub               |   |   `-- usb_hub
> usb           6  [ + ]   ehci_generic          |   |-- usb@1c1d000
> usb_hub       6  [ + ]   usb_hub               |   |   `-- usb_hub
> usb           7  [ + ]   ohci_generic          |   |-- usb@1c1d400
> usb_hub       7  [ + ]   usb_hub               |   |   `-- usb_hub
> 
> The location where a USB is plugged is identified by the port of the USB 
> hub it is connected to.
> 
> I think before we can properly support removable media we will have to 
> get the UEFI device path right. We need a node on all of the levels of 
> the DM tree of which more than one instance can exist.

Here is an example of device path created by an AMI BIOS for an ESP 
partition on a USB stick:

PciRoot(0x0)/Pci(0x1,0x2)/Pci(0x0,0x0)/Pci(0x8,0x0)/Pci(0x0,0x1)/USB(0x2,0x0)/USB(0x3,0x0)/HD(1,GPT,1AD3DE75-504A-47DF-A5CE-81D9EF0252A2,0x40,0x55D824)

The two number in PCI(foo, bar) are:
PCI device
PCI function

The two numbers in USB(foo, bar) are:
USB Parent Port Number
USB Interface Number

By using USB() instead of USBClass() the device path becomes generic. If 
you plug in a device from a different vendor into the same USB port, you 
will get the same device path for the device. Only the partition 
information may change.

Best regards

Heinrich

> 
>>
>> I'm not yet sure where "UsbClass(0x0,0x0,0x9,0x0,0x1)" comes from.
>>
>> "USB(X, Y)", where X is a parent_port_number and Y is a usb_interface,
>> would be a more appropriate candidate for this kind of device path,
>> but we don't utilise this form in the current implementation.
>>
>>> and find the ESP on it at runtime.
>>
>> I don't think the specification requires that the disk is an ESP
>> (at least explicitly).
>>
>>
>>>> => efidebug -b 1 SCSI scsi 0:1
>>>> => efidebug boot dump
>>>> Boot0001:
>>>> attributes: A-- (0x00000001)
>>>>     label: SCSI
>>>>     file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/
>>
>> Contrary to USB, we use Scsi(0,0) for scsi devices but it is NOT 
>> identical
>> to U-Boot's scsi0 (in boot_targets).
>>
>>>>     HD(1,GPT,0ed48d12-1b4c-4e08-b3ee-decf20428036,0x800,0xa000)
>>>>     data:
>>>>       00000000: 00 00
>>>> => efideubg -b 2 USB usb 0:1
>>>
>>> efidebug
>>
>> OK
>>
>> -Takahiro Akashi
>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>> => efidebug boot order 2 1
>>>> => bootefi bootmgr
>>>>
>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>> ---
>>>>    cmd/efidebug.c | 46 ++++++++++++++++++++++++++++++++++++++++------
>>>>    1 file changed, 40 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>>>> index a977ca9c72f5..aaf269cdf47d 100644
>>>> --- a/cmd/efidebug.c
>>>> +++ b/cmd/efidebug.c
>>>> @@ -933,6 +933,29 @@ out:
>>>>        return initrd_dp;
>>>>    }
>>>> +/**
>>>> + * count_arguments - count the number of arguments
>>>> + * @argc:    Total number of arguments
>>>> + * @argv:    Argument array
>>>> + * Return:    Number of arguments
>>>> + *
>>>> + * Count the number of arguments for a given option, "-?"
>>>> + * Specifically if the first argument is not "-?", return 0;
>>>> + */
>>>> +static int count_arguments(int argc, char *const argv[])
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    if (argv[0][0] != '-')
>>>> +        return 0;
>>>> +
>>>> +    for (i = 1; i < argc; i++)
>>>> +        if (argv[i][0] == '-')
>>>> +            break;
>>>> +
>>>> +    return i;
>>>> +}
>>>> +
>>>>    /**
>>>>     * do_efi_boot_add() - set UEFI load option
>>>>     *
>>>> @@ -945,7 +968,7 @@ out:
>>>>     *
>>>>     * Implement efidebug "boot add" sub-command. Create or change 
>>>> UEFI load option.
>>>>     *
>>>> - * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] 
>>>> <file>
>>>> + * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] 
>>>> [<file>]
>>>>     *                   -i <file> <interface2> <devnum2>[:<part>] 
>>>> <initrd>
>>>>     *                   -s '<options>'
>>>>     */
>>>> @@ -979,7 +1002,10 @@ static int do_efi_boot_add(struct cmd_tbl 
>>>> *cmdtp, int flag,
>>>>        argv++; /* 'add' */
>>>>        for (; argc > 0; argc--, argv++) {
>>>>            if (!strcmp(argv[0], "-b")) {
>>>> -            if (argc <  5 || lo.label) {
>>>> +            int num_args;
>>>> +
>>>> +            num_args = count_arguments(argc, argv);
>>>> +            if (num_args <  5 || num_args > 6 || lo.label) {
>>>>                    r = CMD_RET_USAGE;
>>>>                    goto out;
>>>>                }
>>>> @@ -1000,7 +1026,8 @@ static int do_efi_boot_add(struct cmd_tbl 
>>>> *cmdtp, int flag,
>>>>                utf8_utf16_strncpy(&label, argv[2], label_len);
>>>>                /* file path */
>>>> -            ret = efi_dp_from_name(argv[3], argv[4], argv[5],
>>>> +            ret = efi_dp_from_name(argv[3], argv[4],
>>>> +                           num_args == 5 ? "" : argv[5],
>>>>                               &device_path, &file_path);
>>>>                if (ret != EFI_SUCCESS) {
>>>>                    printf("Cannot create device path for \"%s %s\"\n",
>>>> @@ -1008,10 +1035,16 @@ static int do_efi_boot_add(struct cmd_tbl 
>>>> *cmdtp, int flag,
>>>>                    r = CMD_RET_FAILURE;
>>>>                    goto out;
>>>>                }
>>>> +            /* if no file name is given, use device_path */
>>>> +            if (num_args == 5) {
>>>> +                efi_free_pool(file_path);
>>>> +                file_path = device_path;
>>>> +                device_path = NULL;
>>>> +            }
>>>>                fp_size += efi_dp_size(file_path) +
>>>>                    sizeof(struct efi_device_path);
>>>> -            argc -= 5;
>>>> -            argv += 5;
>>>> +            argc -= num_args;
>>>> +            argv += num_args;
>>>>            } else if (!strcmp(argv[0], "-i")) {
>>>>                if (argc < 3 || initrd_dp) {
>>>>                    r = CMD_RET_USAGE;
>>>> @@ -1078,7 +1111,8 @@ out:
>>>>        free(data);
>>>>        efi_free_pool(final_fp);
>>>>        efi_free_pool(initrd_dp);
>>>> -    efi_free_pool(device_path);
>>>> +    if (device_path)
>>>> +        efi_free_pool(device_path);
>>>>        efi_free_pool(file_path);
>>>>        free(lo.label);
>>>>