From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 87ECEC433F5 for ; Tue, 7 Dec 2021 01:34:10 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 4C771830AF; Tue, 7 Dec 2021 02:34:07 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="M+0bhf7e"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 6AAE0830AD; Tue, 7 Dec 2021 02:34:04 +0100 (CET) Received: from mail-pf1-x42b.google.com (mail-pf1-x42b.google.com [IPv6:2607:f8b0:4864:20::42b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 84A08830E1 for ; Tue, 7 Dec 2021 02:33:54 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pf1-x42b.google.com with SMTP id x131so11813300pfc.12 for ; Mon, 06 Dec 2021 17:33:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=O7ezBiVn30xnNGMLakgBKp+xmh08W2LuIpdUB79VCgY=; b=M+0bhf7eY8A3iWfhrP0l/6GV1SPSRdPa7ym1r2BfHUtbD6vK/qbbOJKMkHKvnhTm9M KCqVXqkofwuewm1D2qR9xfKEbpow9Y+/YYSyS8xh4+6TKEkD4hzdORSNR+UUJPeD9gfB moMh8FN/6ZOLPek/b/HiM+sQ5nU5m+8Iu8ccEWLhLxgzWOYks49PtuxsoErqOZyWNsss d4xbLjTKJhSfQij1NGrQudmLm1qv2mdngAdKYZDifwj6uoQiSzntBoZ3w+7p22A89tJY 625YEXXymlDx33t0sYu5megg/gisvzy39nCrv8/Y9saRLWKO8nCCRn6hbO1fU3b4oLho SkwQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=O7ezBiVn30xnNGMLakgBKp+xmh08W2LuIpdUB79VCgY=; b=xlgIa2kng6iLS/URj33e/zrwOE1IO9j8cQjTW7twwI4QfNYfAYWXtydHj9TGgQl1Dr TN/meWV6RwksUsM5LD4mECrsc7AB0SmeLVpn8cEKDdJUr0nNT4rOdG2347GRqenFIual sJIh34WANFfjLowXHSgeD6tiU8qEuvjGfXza6syL8krDvw5g0n4BSNV5vAC2xAnjWuZ9 9tjszggvv/WJaDwq44PsuoaiLleZ7QU9iCNNuZkOtrXO7bdjxM+BgvIASplA43gL3dDI ovvq4w0R4JH42fRjqtRhENt7Odf8BkCkqXSV/oVNManxL7CSzCTIgngD/pd0COxLRsMZ D/YQ== X-Gm-Message-State: AOAM531mLUwbzSjzbanznZ7Yk5l7APd4F1ibv8UNYu5KCgTxj6ogy8t9 VBAPrfni2AGqOevpHdo7EdNGDQ== X-Google-Smtp-Source: ABdhPJxAw+VdNmfU08MtVVx1yyoBI/8fWDUAMRbY8Tvhlnre9Fgv9OVjwAhAm2LNntcvGr7ZHVsCdA== X-Received: by 2002:a63:be4e:: with SMTP id g14mr21539352pgo.194.1638840832594; Mon, 06 Dec 2021 17:33:52 -0800 (PST) Received: from laputa ([2400:4050:c3e1:100:a445:579e:287b:6ec9]) by smtp.gmail.com with ESMTPSA id s3sm638799pjk.41.2021.12.06.17.33.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Dec 2021 17:33:51 -0800 (PST) Date: Tue, 7 Dec 2021 10:33:36 +0900 From: AKASHI Takahiro To: Mark Kettenis Cc: xypron.glpk@gmx.de, sjg@chromium.org, u-boot@lists.denx.de, ilias.apalodimas@linaro.org, agraf@csgraf.de Subject: Re: [BUG] efi_loader: incorrect creation of device paths Message-ID: <20211207013336.GA45273@laputa> Mail-Followup-To: AKASHI Takahiro , Mark Kettenis , xypron.glpk@gmx.de, sjg@chromium.org, u-boot@lists.denx.de, ilias.apalodimas@linaro.org, agraf@csgraf.de References: <6df46f32-f76e-ef2d-aaee-d0349c480789@gmx.de> <20211124031032.GB9598@laputa> <20211125054428.GC41281@laputa> <465784f3-d804-e177-0e34-9213dbcdd436@gmx.de> <20211206041623.GA25701@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.38 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean 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 > > > > 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 > > > > > > > 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