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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8205EC433F5 for ; Sun, 10 Oct 2021 14:14:26 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 4233961058 for ; Sun, 10 Oct 2021 14:14:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 4233961058 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 7AF268364A; Sun, 10 Oct 2021 16:14:21 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="fDd+H+NI"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 0FEB58354E; Sun, 10 Oct 2021 16:14:19 +0200 (CEST) Received: from mail-vk1-xa33.google.com (mail-vk1-xa33.google.com [IPv6:2607:f8b0:4864:20::a33]) (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 2818B833AE for ; Sun, 10 Oct 2021 16:14:14 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-vk1-xa33.google.com with SMTP id bb12so4300508vkb.5 for ; Sun, 10 Oct 2021 07:14:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=G6zGI3y5LNifn+2DRofnJdb/Yoq6gA5M7Y1/xG1jkY8=; b=fDd+H+NI9SHNI+bvbFiF8M+tyJ7VEjAcnr96pMimEnmQdjGIqSSgoWFRTqrgxxgIC8 YZZU5RLxPHSNy+kIZhnOLQzILPCzW04IFHNGY23hjVpLGPAAbfeSKtenomRazduwez7J Es9Yc8XSgD1o/E90zKwllAoeaEAcZCRaQ7tPk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=G6zGI3y5LNifn+2DRofnJdb/Yoq6gA5M7Y1/xG1jkY8=; b=g1vtariR0ohSIq1U92Dip2OtzunRUaDaO8NGytNaGGd7b5u/JAn5rqqSUs3MAa1spO 0QIyUfj7ljBfN+ukNkUUPIOv1CDhWxj6KrpPRosyUnMuuKxQHOk1BfOwouoMKVqUGleH JheL9ySGXa7dk66VFPL5D+JlCaV8EmWMRY+CEk1BvxYmky+AGNNHpyVdwhkBKlCZIIdn fDeBPA/opcf0d8peuADIHuT38cyc4Xu0HensKEy+rcdoc5r3TMzHfE9/tNADUgugMySV YOlHreRjdGvz6fsLWgEDQpPza7El/NlMQbA19YyHmEn1ti+3jPPoLnmgqVhumQD2sEv7 Abcw== X-Gm-Message-State: AOAM532dG+8rS9MhQVOm9Avy48Gu3plXblFveVFgo0hGG063Mbz5Yx4v xHRojLcqAn8cmiHHQWnWoybc6K9xZJnucnuInwmyYA== X-Google-Smtp-Source: ABdhPJw1zmJRoci+I2R+0hQaO5RRO4iHa3E910JfSzha2QUTCWKE4xBu64NnANZth2/1Ev+opuY4s9zHtGuwGTpJBHY= X-Received: by 2002:a1f:2408:: with SMTP id k8mr16987261vkk.11.1633875252084; Sun, 10 Oct 2021 07:14:12 -0700 (PDT) MIME-Version: 1.0 References: <20211001050228.55183-1-takahiro.akashi@linaro.org> In-Reply-To: <20211001050228.55183-1-takahiro.akashi@linaro.org> From: Simon Glass Date: Sun, 10 Oct 2021 08:14:00 -0600 Message-ID: Subject: Re: [RFC 00/22] efi_loader: more tightly integrate UEFI disks to device model To: AKASHI Takahiro Cc: Heinrich Schuchardt , Alex Graf , Ilias Apalodimas , U-Boot Mailing List Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 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 Takahiro, On Thu, 30 Sept 2021 at 23:02, AKASHI Takahiro wrote: > > The purpose of this RPC is to reignite the discussion about how UEFI > subystem would best be integrated into U-Boot device model. > In the past, I poposed a couple of patch series, the latest one[1], > while Heinrich revealed his idea[2], and the approach taken here is > something between them, with a focus on block device handlings. > > # The code is a PoC and not well tested yet. > > Disks in UEFI world: > ==================== > In general in UEFI world, accessing to any device is performed through > a 'protocol' interface which are installed to (or associated with) the device's > UEFI handle (or an opaque pointer to UEFI object data). Protocols are > implemented by either the UEFI system itself or UEFI drivers. > > For block IO's, it is a device which has EFI_BLOCK_IO_PROTOCOL (efi_disk > hereafter). Currently, every efi_disk may have one of two origins: > a.U-Boot's block devices or related partitions > (lib/efi_loader/efi_disk.c) > b.UEFI objects which are implemented as a block device by UEFI drivers. > (lib/efi_driver/efi_block_device.c) > > All the efi_diskss as (a) will be enumelated and created only once at UEFI > subsystem initialization (efi_disk_register()), which is triggered by > first executing one of UEFI-related U-Boot commands, like "bootefi", > "setenv -e" or "efidebug". > EFI_BLOCK_IO_PROTOCOL is implemented by UEFI system using blk_desc(->ops) > in the corresponding udevice(UCLASS_BLK). > > On the other hand, efi_disk as (b) will be created each time UEFI boot > services' connect_controller() is executed in UEFI app which, as a (device) > controller, gives the method to access the device's data, > ie. EFI_BLOCK_IO_PROTOCOL. > > >>> more details >>> > Internally, connect_controller() search for UEFI driver that can support > this controller/protocol, 'efi_block' driver(UCLASS_EFI) in this case, > then calls the driver's 'bind' interface, which eventually installs > the controller's EFI_BLOCK_IO_PROTOCOL to efi_disk object. > 'efi_block' driver also create a corresponding udevice(UCLASS_BLK) for > * creating additional partitions efi_disk's, and > * supporting a file system (EFI_SIMPLE_FILE_SYSTEM_PROTOCOL) on it. > <<< <<< > > Issues: > ======= > 1. While an efi_disk represents a device equally for either a whole disk > or a partition in UEFI world, the device model treats only a whole > disk as a real block device or udevice(UCLASS_BLK). > > 2. efi_disk holds and makes use of "blk_desc" data even though blk_desc > in plat_data is supposed to be private and not to be accessed outside > the device model. > # This issue, though, exists for all the implmenetation of U-Boot > # file systems as well. Yes, this was a migration convenience and we should be able to unpick it now. However we still have SPL_BLK so need to consider whether we can drop that. > > For efi_disk(a), > 3. A block device can be enumelated dynamically by 'scanning' a device bus > in U-Boot, but UEFI subsystem is not able to update efi_disks accordingly. > For examples, > => scsi rescan; efidebug devices > => usb start; efidebug devices ... (A) > (A) doesn't show any usb devices detected. > > => scsi rescan; efidebug boot add -b 0 TEST scsi 0:1 ... > => scsi rescan ... (B) > => bootefi bootmgr ... (C) > (C) may de-reference a bogus blk_desc pointer which has been freed by (B). > (Please note that "scsi rescan" removes all udevices/blk_desc and then > re-create them even if nothing is changed on a bus.) This is something your series will help with. > > For efi_disk(b), > 4. A controller (handle), combined with efi_block driver, has no > corresponding udevice as a parent of efi_disks in DM tree, unlike, say, > a scsi controller, even though it provides methods for block io perations. Can we just create a udevice? > 5. There is no way supported to remove efi_disk's even after > disconnect_controller() is called. > Do we want to remove disks? I don't really understand the context, but if EFI wants to forget about a disk, that should not affect the rest of U-Boot. > > My approach in this RFC: > ======================== > Due to functional differences in semantics, it would be difficult > to identify "udevice" structure as a handle in UEFI world. Instead, we will > have to somehow maintain a relationship between a udevice and a handle. > > 1-1. add a dedicated uclass, UCLASS_PARTITION, for partitions > Currently, the uclass for paritions is not a UCLASS_BLK. > It can be possible to define partitions as UCLASS_BLK > (with IF_TYPE_PARTION?), but > I'm afraid that it may introduce some chaos since udevice(UCLASS_BLK) > is tightly coupled with 'struct blk_desc' data which is still used > as a "structure to a whole disk" in a lot of interfaces. > (I hope that you understand what it means.) > > In DM tree, a UCLASS_PARTITON instance has a UCLASS_BLK parent: > For instance, > UCLASS_SCSI --- UCLASS_BLK --- UCLASS_PARTITION > (IF_TYPE_SCSI) | > +- struct blk_desc +- struct disk_part > +- scsi_blk_ops +- blk_part_ops Seems right to me. But I think a partition should also have a BLK child, so that only block devices can be read/written. In fact, perhaps a partition should be a child of the media device (e.g. MMC), not of the BLK device? That might make more sense. > > 1-2. create partition udevices in the context of device_probe() > part_init() is already called in blk_post_probe(). See the commit > d0851c893706 ("blk: Call part_init() in the post_probe() method"). > Why not enumelate partitions as well in there. Sounds reasonable. > > 2. add new block access interfaces, which takes "udevice" as a target device, > in U-Boot and use those functions to implement efi_disk operations > (i.e. EFI_BLOCK_IO_PROTOCOL). Nice. > > 3-1. maintain a bi-directional link by adding > - a UEFI handle pointer in "struct udevice" > - a udevice pointer in UEFI handle (in fact, in "struct efi_disk_obj") We should generalise the API here, but otherwise seems right. See my comment on the patch. > > 3-2. use device model's post_probe/pre_remove hook to synchronize the lifetime > of efi_disk objects in UEFI world with the device model. We should add an event mechanism to generalise this a bit. See my comment on the patch. > > 4. I have no answer to issue(4) and (5) yet. See above. > > > Patchs: > ======= > For easy understandings, patches may be categorized into seperate groups > of changes. > > Patch#1-#9: DM: create udevices for partitions > Patch#11,#12: UEFI: use udevice interfaces > Patch#13-#16: UEFI: sync up with the device model for adding > Patch#17-#19: UEFI: sync up with the device model for removing > For removal case, we may need more consideration since removing handles > unconditionally may end up breaking integrity of handles > (some may still be held and referenced to by a UEFI app). > Patch#20-#22: UEFI: align efi_driver with changes by the integration > This looks great to me and is the first real step I have seen to actually solving the problem of all this duplication. I think we need a few more DM APIs as I suggest in a few of the patches. I am happy to help with these if you like. I wonder if it is possible to get some version of at least part of this code applied in this merge window? It helps to set things on a much better direction. I also worry that continuing to develop the EFI impl while it is such a sorry state is adding to the difficulty of fixing it up. Regards, Simon > > [1] https://lists.denx.de/pipermail/u-boot/2019-February/357923.html > [2] https://lists.denx.de/pipermail/u-boot/2021-June/452297.html > > AKASHI Takahiro (22): > scsi: call device_probe() after scanning > usb: storage: call device_probe() after scanning > mmc: call device_probe() after scanning > nvme: call device_probe() after scanning > sata: call device_probe() after scanning > block: ide: call device_probe() after scanning > dm: blk: add UCLASS_PARTITION > dm: blk: add a device-probe hook for scanning disk partitions > dm: blk: add read/write interfaces with udevice > efi_loader: disk: use udevice instead of blk_desc > dm: add a hidden link to efi object > efi_loader: remove !CONFIG_BLK code from efi_disk > efi_loader: disk: a helper function to create efi_disk objects from > udevice > dm: blk: call efi's device-probe hook > efi_loader: cleanup after efi_disk-dm integration > efi_loader: add efi_remove_handle() > efi_loader: efi_disk: a helper function to delete efi_disk objects > dm: blk: call efi's device-removal hook > efi_driver: align with efi_disk-dm integration > efi_driver: cleanup after efi_disk-dm integration > efi_selftest: block device: adjust dp for a test disk > (TEST) let dm-tree unchanged after block_io testing is done > > common/usb_storage.c | 6 + > drivers/ata/dwc_ahsata.c | 10 + > drivers/ata/fsl_sata.c | 11 + > drivers/ata/sata_mv.c | 9 + > drivers/ata/sata_sil.c | 12 + > drivers/block/blk-uclass.c | 249 ++++++++++++++++ > drivers/block/ide.c | 6 + > drivers/mmc/mmc-uclass.c | 7 + > drivers/nvme/nvme.c | 6 + > drivers/scsi/scsi.c | 10 + > include/blk.h | 15 + > include/dm/device.h | 4 + > include/dm/uclass-id.h | 1 + > include/efi_loader.h | 8 +- > lib/efi_driver/efi_block_device.c | 30 +- > lib/efi_loader/efi_boottime.c | 8 + > lib/efi_loader/efi_device_path.c | 29 ++ > lib/efi_loader/efi_disk.c | 286 ++++++++++--------- > lib/efi_loader/efi_setup.c | 5 - > lib/efi_selftest/efi_selftest_block_device.c | 28 +- > 20 files changed, 566 insertions(+), 174 deletions(-) > > -- > 2.33.0 >