[BUG] efi_loader: incorrect creation of device paths

Mon Dec 6 12:41:28 CET 2021

> Date: Mon, 6 Dec 2021 13:16:23 +0900
> From: AKASHI Takahiro <takahiro.akashi at linaro.org>
> 
> On Fri, Dec 03, 2021 at 05:32:49PM +0100, Heinrich Schuchardt wrote:
> > On 11/25/21 06:44, AKASHI Takahiro wrote:
> > > Heinrich,
> > > 
> > > On Wed, Nov 24, 2021 at 12:10:32PM +0900, AKASHI Takahiro wrote:
> > > > On Sat, Nov 20, 2021 at 01:54:30PM +0100, Heinrich Schuchardt wrote:
> > > > > Hello Takahiro,
> > > > > 
> > > > > in a prior mail we have discussed the creation of device paths for USB
> > > > > mass storage devices.
> > > > > 
> > > > > On the sand boxyou get the following devices after 'usb start':
> > > > > 
> > > > >   Class     Index  Probed  Driver                Name
> > > > > -----------------------------------------------------------
> > > > >   usb           0  [ + ]   usb_sandbox           |-- usb at 1
> > > > >   usb_hub       0  [ + ]   usb_hub               |   `-- hub
> > > > >   usb_mass_s    0  [ + ]   usb_mass_storage      |       |--
> > > > > usb_mass_storage
> > > > >   blk           3  [   ]   usb_storage_blk       |       |   `--
> > > > > usb_mass_storage.lun0
> > > > >   usb_mass_s    1  [ + ]   usb_mass_storage      |       |--
> > > > > usb_mass_storage
> > > > >   blk           4  [   ]   usb_storage_blk       |       |   `--
> > > > > usb_mass_storage.lun0
> > > > >   usb_mass_s    2  [ + ]   usb_mass_storage      |       `--
> > > > > usb_mass_storage
> > > > >   blk           5  [   ]   usb_storage_blk       |           `--
> > > > > usb_mass_storage.lun0
> > > > > 
> > > > > For of these storage devices we try to create the same device path which
> > > > > is not allowable:
> > > > > 
> > > > > => usb start
> > > > > starting USB...
> > > > > Bus usb at 1: scanning bus usb at 1 for devices... 5 USB Device(s) found
> > > > >         scanning usb for storage devices... 3 Storage Device(s) found
> > > > > =>
> > > > > => efidebug dh
> > > > > Scanning disk mmc2.blk...
> > > > > handle 0000000015e34f00,
> > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(2)/SD(0)
> > > > > Scanning disk mmc1.blk...
> > > > > handle 0000000015e36b30,
> > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(1)/SD(1)
> > > > > Scanning disk mmc0.blk...
> > > > > handle 0000000015e35b00,
> > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(2)
> > > > > Scanning disk usb_mass_storage.lun0...
> > > > > handle 0000000015e35c10,
> > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x1234,0x5678,0x9,0x0,0x0)/UsbClass(0x1234,0x5678,0x0,0x0,0x0)
> > > > > fs_devread read outside partition 2
> > > > > Failed to mount ext2 filesystem...
> > > > > BTRFS: superblock end 69632 is larger than device size 512
> > > > > Scanning disk usb_mass_storage.lun0...
> > > > > handle 0000000015e361f0,
> > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x1234,0x5678,0x9,0x0,0x0)/UsbClass(0x1234,0x5678,0x0,0x0,0x0)
> > > > > ERROR: failure to add disk device usb_mass_storage.lun0, r = 20
> > > > > Error: Cannot initialize UEFI sub-system, r = 20
> > > > > 
> > > > > I will provide a patch that will allow the first USB device to be used
> > > > > and avoids stopping the boot process. But we really have to walk the dm
> > > > > tree to create a device patch for USB devices based on port IDs.
> > > > > 
> > > > > We should add a new field to struct uclass_driver:
> > > > > 
> > > > > struct efi_device_path *get_node(udevice *dev);
> > > > > 
> > > > > This function shall return a pointer to an freshly allocated buffer with
> > > > > the device node for the device. To build the devicepath we can then walk
> > > > > the dm tree.
> > > > 
> > > > I'm not sure this is an acceptable solution.
> > > > 
> > > > Let me make sure:
> > > > The goal would be to create a device path like
> > > >     .../USB(0x1,0x0)/HD(1,...)
> > > > instead of
> > > >     .../UsbHub(0x0,0x0,0x0,0x3)/UsbMassStorage(0x46f4,0x1,0x0,0x0)/HD(1,...)
> > > > as we already see this format for SCSI:
> > > >     .../Scsi(0,0)/HD(1,..)
> > > > 
> > > > Right?
> > > 
> > > Please try the tweak attached below.
> > > (I think what I did here is trivial.)
> > > 
> > > If you like, I will post it as a patch.
> > > (See "10.3.4.5.1 USB Device Path Example".)
> > > 
> > > -Takahiro Akashi
> > > 
> > > > > >  From cda91e9d8144f89f0d73738b338289a7940bbe0e Mon Sep 17 00:00:00 2001
> > > > > > From: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > > > > > Date: Thu, 25 Nov 2021 14:20:08 +0900
> > > > > > Subject: [PATCH] efi_loader: disk: usb mass-storage
> > > 
> > > - use path_usb instead of path_usb_class for existing USB dp nodes
> > > - add a dp node for UCLASS_USB
> > > 
> > > => usb start
> > > starting USB...
> > > Bus xhci_pci: Register 8001040 NbrPorts 8
> > > Starting the controller
> > > USB XHCI 1.00
> > > scanning bus xhci_pci for devices... 4 USB Device(s) found
> > >         scanning usb for storage devices... 2 Storage Device(s) found
> > > => dm tree
> > >   Class     Index  Probed  Driver                Name
> > > -----------------------------------------------------------
> > >   root          0  [ + ]   root_driver           root_driver
> > >   ...
> > >   pci           0  [ + ]   pci_generic_ecam      |-- pcie at 10000000
> > >   pci_generi    0  [   ]   pci_generic_drv       |   |-- pci_0:0.0
> > >   virtio       32  [ + ]   virtio-pci.l          |   |-- virtio-pci.l#0
> > >   ethernet      0  [ + ]   virtio-net            |   |   `-- virtio-net#32
> > >   usb           0  [ + ]   xhci_pci              |   `-- xhci_pci
> > >   usb_hub       0  [ + ]   usb_hub               |       `-- usb_hub
> > >   usb_dev_ge    0  [ + ]   usb_dev_generic_drv   |           |-- generic_bus_0_dev_2
> > >   usb_mass_s    0  [ + ]   usb_mass_storage      |           |-- usb_mass_storage
> > >   blk           0  [   ]   usb_storage_blk       |           |   `-- usb_mass_storage.lun0
> > >   usb_mass_s    1  [ + ]   usb_mass_storage      |           `-- usb_mass_storage
> > >   blk           1  [   ]   usb_storage_blk       |               `-- usb_mass_storage.lun0
> > >   ...
> > > => efidebug devices
> > > Scanning disk usb_mass_storage.lun0...
> > > Scanning disk usb_mass_storage.lun0...
> > > Found 6 disks
> > > Missing RNG device for EFI_RNG_PROTOCOL
> > > ** Unable to read file ubootefi.var **
> > > Failed to load EFI variables
> > > Unable to find TPMv2 device
> > > Device           Device Path
> > > ================ ====================
> > > 000000013eef3a50 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> > > 000000013eefe840 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x2,0x0)
> > > 000000013eefe990 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x2,0x0)/HD(1,0x01,0,0x0,0x99800)
> > > 000000013eefeae0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x2,0x0)/HD(2,0x01,0,0x99800,0x1800)
> > > 000000013eeff5f0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x3,0x0)
> > > 000000013eeff990 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x3,0x0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a)
> > > 000000013eeffda0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x3,0x0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800)
> > 
> > I am missing a PCI() node here for the USB root Hub. In the DM tree: Is
> > xhci_pci the root hub and usb_hub a hub connected to this root hub? Or
> > do both nodes together model a root hub? Anyway USB(0xf3,0x0) seems to
> > be wrong. There is no USB hub with a port 243.
> 
> As the code suggests, the path was generated this way:
> 
> /VenHw(...)/
> USB(0xf3,0x0)/  <= usb_controller (xhci_pci)
> USB(0x0,0x0)/   <= usb_hub
> USB(0x3,0x0)/   <= usb_mass_storage
> HD(1,GPT,...)
> 
> USB(0xf3,0x0) was incorrectly encoded here, instead we will be able
> to generate PCI(xx,yy) by examining
>    struct pci_child_plat *pplat = dev_get_parent_plat(dev);
>    xx = PCI_DEV(pplat->devfn), yy = PCI_FUNC(pplat->devfn)
> if the dev's parent is a PCI (controller).
> 
> But we will have to confirm that this is the only case that
> we need to take care of the parent's bus.
> Please note that any kind of pci component is not encoded
> in a device path under the current UEFI implementation on U-Boot.

You'll need to take into account any PCI-PCI bridges, so you need to
do this in a loop, walking up the tree until you encounter the PCI
host bridge / root complex ("PciRoot").

The stadard also is of the opinion that PciRoot needs to be an ACPI
device path entry, which obviously doesn't make sense in a DT context.
Using VenHW makes more sense, but maybe UEFI needs to grow support for
proper DT device path entries...

> > Please, see the examples on p. 303ff of the UEFI 2.9 specification:
> > 
> > PciRoot(0)/PCI(31,2)/USB(0,0)
> > PciRoot(0)/PCI(31,2)/USB(1,0)/USB(3,0)
> > 
> > The first example is a USB controller connected to port 0 of the root hub.
> > The second is a USB controller connected to port 3 of a USB hub which is
> > connected to port 1 of the root hub.
> 
> So do you agree that this is a right approach even if it will break
> some sort of compatibility (of device path representation) with the current
> U-Boot UEFI implementation?

This will break any Boot#### variables that have been set by users
isn't it?  I guess there is no way to avoid that, but it probably is a
good idea to only break it once.