Re: [BUG] efi_loader: incorrect creation of device paths

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Hi Mark,

Thank you for the comment.

On Mon, Dec 06, 2021 at 12:41:28PM +0100, Mark Kettenis wrote:
> > Date: Mon, 6 Dec 2021 13:16:23 +0900
> > From: AKASHI Takahiro <takahiro.akashi@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@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@1: scanning bus usb@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@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@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").

Yes, you're right, but the current U-Boot's DM framework doesn't
model the root complex. Instead, calling pci_get_controller() would
be enough. (Need confirm this.)

        -> Simon?

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

Yeah, the current VenHW(e61d73b9-a384-4acc-aeab-82e828f3628b) represents
"efi_root" in our UEFI implementation and it is another incompatibility
with other UEFI firmware.

After all, we might want to insert a dummy PciRoot(0) (as EDK2 does?).


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

Indeed.

-Takahiro Akashi