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 A3CEAC0015E for ; Thu, 20 Jul 2023 00:14:48 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id B26088679F; Thu, 20 Jul 2023 02:14:46 +0200 (CEST) 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="T12Rx+2c"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 27249867A0; Thu, 20 Jul 2023 02:14:46 +0200 (CEST) Received: from mail-pl1-x62a.google.com (mail-pl1-x62a.google.com [IPv6:2607:f8b0:4864:20::62a]) (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 D204F8679E for ; Thu, 20 Jul 2023 02:14:42 +0200 (CEST) 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-pl1-x62a.google.com with SMTP id d9443c01a7336-1b8c364ad3bso356055ad.1 for ; Wed, 19 Jul 2023 17:14:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1689812081; x=1690416881; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=ojpwGXPMRg0DR1o6hgQHQ8/kFbQ5u+/pwSjkBc9K880=; b=T12Rx+2cRlKtvTkl1O7+k6JT6HxolFbNlPRCrlW0C8uFZFDFjAPrM/IU5aC8Acfxdm VzGxMmolnL+lKi+TxgDF/CbD+SCsnW4oXruWOISu0vXZxvcgrMk5bKM46ZPHCsAABrJs RJA0YMk2cYvfI6R4ohLOzygOrNPpJXvicG6cfxUFQdk8ZxJqK+wIhYj824+qEb3n3ctb y6JKjPmthleN+THv04brqdEJDeBWhPwLJ8HwsFsAn5xvC9v3hggQPRJAn3WsdhI+58rt ewsdpJM6J4EHaPOTgWqQw17nBX883xRb2luD/7MqyHGdIUyyo1UeJcUFj4oZCEyYjeSh 9/Pg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689812081; x=1690416881; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ojpwGXPMRg0DR1o6hgQHQ8/kFbQ5u+/pwSjkBc9K880=; b=dcTRtEzfXqNG6SGE/en4h0nPhes1G81zpIr/RjU9gEz6IXZ9Rs47gE+V50/Fn+D4sV d+/IVJwxxilN63MakmJ+laHyrpnjOlqBO1oZthfdgcWGHgY6zUvWYAgn+r4bUC+TjRZ6 i1QspgXEX9dpj9SEgTll/mx2HCwIMFj1CQIEcMJk/tcWt1S3qo++S4at54lFdHObLu8z /o2Kmw8xnRHARAvtNrNQgi418oLItVizpwIP5kfh4gVZhEzn42SYep+mnfyUe5EjU77K VuOSAAwS1jMDe+XqwtOgnOgySBgNvxpUCYCNkYUBKjPByI4B7mmW2NAzG4USOfmBio1F f/pQ== X-Gm-Message-State: ABy/qLaqKN2grbSWjQslbVyHLOZeKuGtoza/FsL0tRTgkfIzOor3F4N2 UlJnY/vbh8HSnqWZPdK7RpGZ8w== X-Google-Smtp-Source: APBJJlH6Wh7N9gbZWVTpAh2hrV0yfIdThvrKupFRudpNYypghWFCY5/cvdBA1xcNFoVk0teEuoXCFg== X-Received: by 2002:a17:902:e809:b0:1b8:50a9:6874 with SMTP id u9-20020a170902e80900b001b850a96874mr1475036plg.5.1689812080849; Wed, 19 Jul 2023 17:14:40 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:b19f:5d18:5b09:a6df]) by smtp.gmail.com with ESMTPSA id z15-20020a1709028f8f00b001b5656b0bf9sm4525069plo.286.2023.07.19.17.14.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 19 Jul 2023 17:14:39 -0700 (PDT) Date: Thu, 20 Jul 2023 09:14:36 +0900 From: AKASHI Takahiro To: Heinrich Schuchardt Cc: Simon Glass , masahisa.kojima@linaro.org, u-boot@lists.denx.de Subject: Re: [RFC] efi_driver: fix a parent issue in efi-created block devices Message-ID: Mail-Followup-To: AKASHI Takahiro , Heinrich Schuchardt , Simon Glass , masahisa.kojima@linaro.org, u-boot@lists.denx.de References: <20230719002158.27004-1-takahiro.akashi@linaro.org> <30f22c59-c6ce-1cea-9ef0-bdc31f7d57c2@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <30f22c59-c6ce-1cea-9ef0-bdc31f7d57c2@gmx.de> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 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.8 at phobos.denx.de X-Virus-Status: Clean On Wed, Jul 19, 2023 at 03:15:10PM +0200, Heinrich Schuchardt wrote: > On 19.07.23 15:04, Simon Glass wrote: > > Hi, > > > > On Tue, 18 Jul 2023 at 19:54, AKASHI Takahiro > > wrote: > > > > > > Hi Simon, > > > > > > On Tue, Jul 18, 2023 at 07:08:45PM -0600, Simon Glass wrote: > > > > Hi AKASHI, > > > > > > > > On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro > > > > wrote: > > > > > > > > > > An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in > > > > > EFI world, which in turn generates a corresponding U-Boot block device based on > > > > > U-Boot's Driver Model. > > > > > The latter device, however, doesn't work as U-Boot proper block device > > > > > due to an issue in efi_driver's implementation. We saw discussions in the past, > > > > > most recently in [1]. > > > > > > > > > > [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html > > > > > > > > > > This RFC patch tries to address (part of) the issue. > > > > > If it is considered acceptable, I will create a formal patch. > > > > > > > > > > Withtout this patch, > > > > > ===8<=== > > > > > => env set efi_selftest 'block device' > > > > > => bootefi selftest > > > > > ... > > > > > > > > > > Summary: 0 failures > > > > > > > > > > => dm tree > > > > > Class Index Probed Driver Name > > > > > ----------------------------------------------------------- > > > > > root 0 [ + ] root_driver root_driver > > > > > ... > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > => ls efiloader 0:1 > > > > > ** Bad device specification efiloader 0 ** > > > > > Couldn't find partition efiloader 0:1 > > > > > ===>8=== > > > > > > > > > > With this patch applied, efiblk#0(:1) now gets accessible. > > > > > > > > > > ===8<=== > > > > > => env set efi_selftest 'block device' > > > > > => bootefi selftest > > > > > ... > > > > > > > > > > Summary: 0 failures > > > > > > > > > > => dm tree > > > > > Class Index Probed Driver Name > > > > > ----------------------------------------------------------- > > > > > root 0 [ + ] root_driver root_driver > > > > > ... > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > => ls efiloader 0:1 > > > > > 13 hello.txt > > > > > 7 u-boot.txt > > > > > > > > > > 2 file(s), 0 dir(s) > > > > > ===>8=== > > > > > > > > > > Signed-off-by: AKASHI Takahiro > > > > > --- > > > > > include/efi_driver.h | 2 +- > > > > > lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- > > > > > lib/efi_driver/efi_uclass.c | 8 +++++++- > > > > > lib/efi_selftest/efi_selftest_block_device.c | 2 ++ > > > > > 4 files changed, 22 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/include/efi_driver.h b/include/efi_driver.h > > > > > index 63a95e4cf800..ed66796f3519 100644 > > > > > --- a/include/efi_driver.h > > > > > +++ b/include/efi_driver.h > > > > > @@ -42,7 +42,7 @@ struct efi_driver_ops { > > > > > const efi_guid_t *child_protocol; > > > > > efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); > > > > > efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, > > > > > - efi_handle_t handle, void *interface); > > > > > + efi_handle_t handle, void *interface, char *name); > > > > > }; > > > > > > > > > > #endif /* _EFI_DRIVER_H */ > > > > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > > > > > index add00eeebbea..43b7ed7c973c 100644 > > > > > --- a/lib/efi_driver/efi_block_device.c > > > > > +++ b/lib/efi_driver/efi_block_device.c > > > > > @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, > > > > > * Return: status code > > > > > */ > > > > > static efi_status_t > > > > > -efi_bl_create_block_device(efi_handle_t handle, void *interface) > > > > > +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) > > > > > { > > > > > - struct udevice *bdev = NULL, *parent = dm_root(); > > > > > + struct udevice *bdev = NULL; > > > > > efi_status_t ret; > > > > > int devnum; > > > > > char *name; > > > > > @@ -181,7 +181,7 @@ err: > > > > > */ > > > > > static efi_status_t efi_bl_bind( > > > > > struct efi_driver_binding_extended_protocol *this, > > > > > - efi_handle_t handle, void *interface) > > > > > + efi_handle_t handle, void *interface, char *name) > > > > > { > > > > > efi_status_t ret = EFI_SUCCESS; > > > > > struct efi_object *obj = efi_search_obj(handle); > > > > > @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( > > > > > if (!obj || !interface) > > > > > return EFI_INVALID_PARAMETER; > > > > > > > > > > - if (!handle->dev) > > > > > - ret = efi_bl_create_block_device(handle, interface); > > > > > + if (!handle->dev) { > > > > > + struct udevice *parent; > > > > > + > > > > > + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); > > > > > > > > Can you use a non-root device as the parent? > > > > > > I have no idea what else can be the parent in this case. > > > > I suggest an EFI_MEDIA device. > > > > > > > > Please note: > > > > > => dm tree > > > > > Class Index Probed Driver Name > > > > > ----------------------------------------------------------- > > > > > root 0 [ + ] root_driver root_driver > > > > > ... > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > > > This "efi" object is created by an EFI application (i.e. efi_selftest_block_device.c) > > > and don't have any practical parent. > > > > Block devices must have a media device as their parent. This seems to > > be a persistent area of confusion...probably when the uclass ID goes > > away from blk_desc it will be more obvious. > > Dear Simon, > > The only reason why you request to add an otherwise parent device is > that you use it to determine the device class name used in the CLI (mmc, > usb, nvme, ...). > > That concept worked fine when all devices had physical parents from > which such an information could be derived. > > This is not the case UCLASS_EFI block devices. We should not introduce > any DM devices which have no meaning in the EFI world. Regarding my RFC patch, I have not invented any new DM device, instead I reuse the existing one, UCLASS_EFI_LOADER, which strangely never appears in DM tree under the current implementation. With my patch, a new instance (device) is created and associated with a "controller handle" (in UEFI jargon) which is passed on to EFI_DRIVER_BINDING_PROTOCOL.start() by a UEFI app. So the hierarchy looks like: ROOT UCLASS_EFI_LOADER - controller UCLASS_BLK - raw device UCLASS_PARTITION - partition It seems to me that it perfectly matches to DM concept. It has nothing different from other ordinary block devices. > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) The guid here is exactly what you gave to the controller handle (disk_handle) in your lib/efi_selftest/efi_selftest_block_device.c. -Takahiro Akashi > If there is no other benefit, we should do the reasonable and keep a > field in blk_desc and use it to derive the CLI name of the block device. > > Best regards > > Heinrich > > > > > > > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > > > > > > > + if (!ret) > > > > > + ret = efi_bl_create_block_device(handle, interface, parent); > > > > > + else > > > > > + ret = EFI_DEVICE_ERROR; > > > > > + } > > > > > > > > > > return ret; > > > > > } > > > > > diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c > > > > > index 45f935198874..bf669742783e 100644 > > > > > --- a/lib/efi_driver/efi_uclass.c > > > > > +++ b/lib/efi_driver/efi_uclass.c > > > > > @@ -145,7 +145,13 @@ static efi_status_t EFIAPI efi_uc_start( > > > > > ret = check_node_type(controller_handle); > > > > > if (ret != EFI_SUCCESS) > > > > > goto err; > > > > > - ret = bp->ops->bind(bp, controller_handle, interface); > > > > > + > > > > > + struct efi_handler *handler; > > > > > + char tmpname[256] = "AppName"; > > > > > + ret = efi_search_protocol(controller_handle, &efi_guid_device_path, > > > > > + &handler); > > > > > + snprintf(tmpname, 256, "%pD", handler->protocol_interface); > > > > > + ret = bp->ops->bind(bp, controller_handle, interface, strdup(tmpname)); > > > > > if (ret == EFI_SUCCESS) > > > > > goto out; > > > > > > > > > > diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c > > > > > index a367e8b89d17..0ab8e4590dfe 100644 > > > > > --- a/lib/efi_selftest/efi_selftest_block_device.c > > > > > +++ b/lib/efi_selftest/efi_selftest_block_device.c > > > > > @@ -248,6 +248,7 @@ static int teardown(void) > > > > > { > > > > > efi_status_t r = EFI_ST_SUCCESS; > > > > > > > > > > +#if 0 /* Temporarily out for confirmation */ > > > > > if (disk_handle) { > > > > > r = boottime->uninstall_protocol_interface(disk_handle, > > > > > &guid_device_path, > > > > > @@ -273,6 +274,7 @@ static int teardown(void) > > > > > return EFI_ST_FAILURE; > > > > > } > > > > > } > > > > > +#endif > > > > > return r; > > > > > } > > > > > > > > > > -- > > > > > 2.41.0 > > > > > > > > > > > > > Otherwise this looks good to me. We should have DM devices for all EFI > > > > ones (in fact EFI ones should just be some extra data on top of DM > > > > ones). > > > > > > Unfortunately, in this specific case (efi_block_device.c), UEFI object > > > (handle) is set to be created first, then U-Boot device (efiblk#xxx). > > > So "some extra data on top of DM ones" is not accurate (doesn't reflect > > > the current implementation). > > > > OK, so we should really sort that out :-) > > > > > > > > Please note again that efi_loader/efi_disk.c and efi_driver/efi_block_device.c > > > are totally different things. > > > > OK > > > > Regards, > > Simon >